cols_is_pk variants by fluca1978 · Pull Request #178 · theory/pgtap

fluca1978 · 2018-10-02T15:20:58Z

This is a potential implementation of #133, in particular col_is_pk(name, name, name).
I'm not sure this is something really required, because there is a potential clash with a textual overloaded function: col_is_pk( name, name, name) requires explicit cast to not clahs with col_is_pk( name, name, description).
I'm not able to decide which is best approach, but sounds to me that col_is_pk( 'public', 'foo', 'id' ) is simpler than col_is_pk( 'public', 'foo', 'id'::name ). Therefore I'm not sure this pull request is the right implementation, and before digging into 'isnt_col_pk' set of functions I would like comments.

This allows for the automatic overloading of the function, implementing therefore the col_is_pk( name, name, name[] ) as requested in issue theory#133. See issue theory#133.

format() has been introduced with 9.1 or so, and using it to easiliy compose messages produces errors in compatibility with older versions.

Due to the addition of variants methods the output of the test has changed.

theory

Yeah, if it is likely to break existing tests that don't have a cast, I'm not sure adding a new test function makes sense. I like supporting as many variants as possible, and am sad that I missed some when I originally wrote this thing.

OTOH, if it's the new variant that would require casting, and the old variant continued to work, that would be just fine. We have to decide what to do about DEFAULT and older versions of Postgres, though.

theory · 2018-11-10T19:12:47Z

sql/pgtap.sql.in

 -- col_is_pk( schema, table, column, description )
-CREATE OR REPLACE FUNCTION col_is_pk ( NAME, NAME, NAME[], TEXT )
+-- col_is_pk( schema, table, column )
+CREATE OR REPLACE FUNCTION col_is_pk ( NAME, NAME, NAME[], TEXT DEFAULT NULL )


Omit DEFAULT until support for 8.3 is dropped.

theory · 2018-11-10T19:14:12Z

sql/pgtap.sql.in

+CREATE OR REPLACE FUNCTION col_is_pk ( NAME, NAME, NAME[], TEXT DEFAULT NULL )
 RETURNS TEXT AS $$
-    SELECT is( _ckeys( $1, $2, 'p' ), $3, $4 );
+    SELECT is( _ckeys( $1, $2, 'p' ), $3,


Without support for DEFAULT, the pattern up to now has been to create separate functions for each signature. Unless @decibel or someone finishes the update to remove support for 9.0 and earlier, I think we need to stick to that pattern.

Got it, I'll try to refactor as soon as I've time.

master is no longer testing < 9.1, so I think DEFAULT is ok now. It'd be good to merge master though since it now has much more extensive automated testing.

nasbyj · 2019-11-19T19:47:31Z

test/sql/pktap.sql

+);
+
+SELECT * FROM check_test(
+    col_is_pk( 'sometab', 'id'::name, 'sometab.id should be a pk' ),


Why is the name cast needed here? AFAICT requiring users to add that would break backwards compatibility, which I don't think we can do :(

Unfortunately, https://travis-ci.org/theory/pgtap/jobs/614184912 shows that we have a problem when removing this cast. :(

One possible way to handle this would be C code, since we could then use one of the polymorphic pseudotypes. I'm not sure this feature is worth that though.

We could possibly have a config option to make to provide the old behavior.

Another possibility would be a version of the function that checked the first argument to see if it was a schema vs a table and acted accordingly (as well as throwing the correct error if neither existed). Before trying to code that we'd want to make sure we have full test cases for all the possible "does not exist" cases, both with and without a description provided. This would be unfortunately complex, but I think it's doable.

Well, or we could just tell people we're breaking backwards compatibility... but if we wanted to do that I think we'd need to do a 2.0.

This is still an issue as well.

In current versions users do not need to cast, so we need to test without a cast.

Take back the work on this Pull Request, see comment <theory#178 (comment)>

…pk_variants

fluca1978 · 2019-11-22T12:30:23Z

I've done some moneky typing merging the master branch, and make installcheck passes all the tests right now, at least against version 12.
Since it has passed a lot of time, please advice if there is some more work to be done.

nasbyj · 2019-11-22T20:34:58Z

Looks like the ALTER EXTENSION UPDATE tests are failing (https://travis-ci.org/theory/pgtap/builds/615528764); is your latest code in sql/pgtap--1.0.0--1.1.0.sql?

fluca1978 · 2019-11-23T09:31:55Z

Seems to me my branch is updated as commit 950c170 which does me the pktap test passing:

% pg_prove test/sql/pktap.sql 
test/sql/pktap.sql .. ok     
All tests successful.
Files=1, Tests=87,  1 wallclock secs ( 0.04 usr  0.00 sys +  0.01 cusr  0.00 csys =  0.05 CPU)
Result: PASS

I always get confused about travis, so I'm probably doing something wrong in my development platform, but I cannot see what.

nasbyj · 2019-11-23T23:23:13Z

Ahh, I understand your confusion then. You're showing the results of running a single test (essentially, what make test does). The Travis suite runs that plus many more tests. With the latest set of results (https://travis-ci.org/theory/pgtap/builds/615528764), by looking at that PGVERSION is set to you can see that your branch is failing on Postgres 9.3-11. Click on the first failure on that page (UPGRADE_TO=9.4 PGVERSION=9.3), and then scroll to the very bottom and you'll see there's 3 tests that are failing. However, that's not the entire story: Line 1044 of the results is this:

Makefile:329: recipe for target 'updatecheck' failed

That means that the failure happened while running make updatecheck. That test works by downloading an old version of pgTap (by default, 0.95.0), installing it, doing ALTER EXTENSION UPDATE, and then running the test suite. This test is designed to catch problems with the upgrade script. This makes me think that the code that you have in pgtap.sql.in is out of sync with what's in pgtap--1.0.0--1.1.0.sql.

All that said, there's a bigger issue: your change will force users to add a cast in col_is_pk(table, column, description) case, which breaks backwards compatibility, which I don't think we can do.

fluca1978 · 2020-01-22T11:00:27Z

I'm having quite an hard time in finding resources to work on this.
If someone would like to continue this work, because at the moment I don't know when I will be able to finish it.

theory · 2022-05-21T23:18:51Z

I pushed 8f4a881, an updated variant of #199, and then noticed this PR implementing the same feature, so I added thanks in ff4a30b. Super appreciate your work on this, @fluca1978!

fluca1978 added 6 commits October 2, 2018 17:15

Refactoring of the function col_is_pk to accept a null test description.

ec9ed8c

This allows for the automatic overloading of the function, implementing therefore the col_is_pk( name, name, name[] ) as requested in issue theory#133. See issue theory#133.

Tests for new col_is_pk implementations.

14144f3

Update documentation with the new implemented functions col_is_pk

c1cff6b

Remove usage of format().

1d07f2e

format() has been introduced with 9.1 or so, and using it to easiliy compose messages produces errors in compatibility with older versions.

Fixed expected output from pktap test.

38cff6c

Due to the addition of variants methods the output of the test has changed.

Forgotten pktap test boundaries in expected output.

40e04ce

theory requested changes Nov 10, 2018

View reviewed changes

theory added the enhancement label Nov 20, 2018

decibel force-pushed the master branch 2 times, most recently from 2b4e6b8 to 0322ca7 Compare February 21, 2019 23:10

chamini2 mentioned this pull request Apr 3, 2019

add non-description versions of col_is_pk with schema #199

Closed

theory approved these changes Nov 14, 2019

View reviewed changes

Merge branch 'master' into col_is_pk_variants

ad48eb2

nasbyj reviewed Nov 19, 2019

View reviewed changes

nasbyj and others added 4 commits November 19, 2019 13:53

Add changes to upgrade script

b14fc25

Remove cast in test for col__is_pk( table, column, description )

6e95404

In current versions users do not need to cast, so we need to test without a cast.

Merge branch 'master' into col_is_pk_variants

4b17767

Take back the work on this Pull Request, see comment <theory#178 (comment)>

Merge remote-tracking branch 'github/col_is_pk_variants' into col_is_…

950c170

…pk_variants

theory added the waiting on author label Oct 10, 2021

theory force-pushed the master branch from ec3c469 to 472808e Compare November 5, 2021 20:57

theory closed this May 21, 2022

Conversation

fluca1978 commented Oct 2, 2018

Uh oh!

theory left a comment

Choose a reason for hiding this comment

Uh oh!

theory Nov 10, 2018

Choose a reason for hiding this comment

Uh oh!

theory Nov 10, 2018

Choose a reason for hiding this comment

Uh oh!

fluca1978 Nov 21, 2018

Choose a reason for hiding this comment

Uh oh!

nasbyj Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

nasbyj Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

nasbyj Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

nasbyj Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

fluca1978 commented Nov 22, 2019

Uh oh!

nasbyj commented Nov 22, 2019

Uh oh!

fluca1978 commented Nov 23, 2019

Uh oh!

nasbyj commented Nov 23, 2019

Uh oh!

fluca1978 commented Jan 22, 2020

Uh oh!

theory commented May 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

theory commented May 21, 2022 •

edited

Loading