Fwd: pgtap identity error

[kevin]

How do we test "IDENTITY" columns? I thought I'd use those since they're supposed to be more standard than SERIAL, but I can't find anything about them in the pgtap docs. I have a table like:

 

CREATE TABLE jj.history (

    history_pk bigint   NOT NULL   PRIMARY KEY   GENERATED ALWAYS AS IDENTITY,

…

);

 

db=# \d jj.history

                                        Table "jj.history"

   Column   |              Type              | Collation | Nullable |           Default

------------+--------------------------------+-----------+----------+------------------------------

history_pk | bigint                         |           | not null | generated always as identity

 

pg_dump says:

 

CREATE TABLE jj.history (

    history_pk bigint NOT NULL,

…

);

 

ALTER TABLE jj.history ALTER COLUMN history_pk ADD GENERATED ALWAYS AS IDENTITY (

    SEQUENCE NAME jj.history_history_pk_seq

    START WITH 1

    INCREMENT BY 1

    NO MINVALUE

    NO MAXVALUE

    CACHE 1

);

 

With pgtap I do:

 

SELECT has_sequence('jj', 'history_history_pk_seq', 'has history_history_pk_seq');

SELECT col_default_is('jj', 'history', 'history_pk', 'generated always as identity', 'history_pk default');

 

pg_prove -v gives me:

 

ok 15 - has history_history_pk_seq

not ok 16 - history_pk default

# Failed test 16: "history_pk default"

#     Column jj.history.history_pk has no default

 

Please note the "has no default" in the error message. Does this mean pgtap doesn't know how to deal with IDENTITY yet or that I'm doing it wrong? If I'm wrong, what is the right way to verify the identity part is there and is the correct form (always -v- default)?

 

This is with Pg 12.7 and pgtap 1.1.0 (from Ubuntu 20.04.02 packages).

 

Thanks,

Kevin

[David E. Wheeler]

On Aug 12, 2021, at 2:53 PM, kevin <kbra...@pwhome.com> wrote:

> How do we test "IDENTITY" columns? I thought I'd use those since they're supposed to be more standard than SERIAL, but I can't find anything about them in the pgtap docs. I have a table like:
>
> CREATE TABLE jj.history (
> history_pk bigint NOT NULL PRIMARY KEY GENERATED ALWAYS AS IDENTITY,
> …
> );

Oh that’s a new feature I hadn’t even noticed. Anyone want to take a stab at adding support for this constraint (attribute?) to pgTAP?

Best,

David

[kevin]

I haven't exhausted all options yet, but I did a "psql -E -c '\d history'" and then worked thru all of the sql returned and I wasn't able to find that constraint in the pg_* tables. I probably need to look a little harder, then post to the "pg general" forum if I still fail to figure it out ... or I'd think that that'd the first step to adding something like this.

I'll see if I can find some time soon to do that. Any hints on existing code to look at? I'd guess the "col_default_is" function, but anything else?

Kevin

[Jim Nasby]

On 8/22/21 7:52 PM, kevin wrote:

> Any hints on existing code to look at? I'd guess the "col_default_is"
> function, but anything else?

Since there's 2 different forms of generated columns, with the AS
IDENTITY version taking 2 options plus sequence options, I think we need
something more sophisticated than a single function (like
col_default_is). Just listing out all the things you might want to check...

1) Is the column GENERATED ... STORED or GENERATED ... AS IDENTITY?

2) If STORED, what's the expression?

3) If AS IDENTITY, is it ALWAYS or BY DEFAULT?

4) If AS IDENTITY, what are the sequence options (if any)?

We can skip #1 if we make sure that 2-4 ensure the generated column is
the right kind.

As for where this info is in the catalogs, pg_attribute has some info; I
suspect you'll need to join that to pg_depends to see where other stuff
is located.

[kevin]

Thanks, I'll check those out. Maybe I can try to get that done tomorrow ... maybe.

I'm not sure about "STORED", where are you seeing that? Did you mean ALWAYS -v- BY DEFAULT? When I look at the doc for "create table" I only see:

	GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( sequence_options ) ]

Do we really need the sequence options? I can kinda sorta see it each way. Regardless of the "sequence options" (which I'd love some comments on), col_default_is() definitely needs to be changed.

Kevin

[kevin]

On 8/24/21 10:40 PM, kevin wrote:
>
> I'm not sure about "STORED", where are you seeing that? Did you mean
> ALWAYS -v- BY DEFAULT? When I look at the doc for "create table" I
> only see:

> GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ (/|sequence_options|/ ) ]
>

My apologies Jim, I found the "stored" version when I looked this
morning. It was a plain as day. I don't know why I didn't see it last night.

Anyway, I have a query gives us pretty much everything we need as this
example shows:

create table k1 (

id   serial not null primary key,

x    int,

i_bd int generated by default as identity,

i_a  int generated always as identity,

i_as int generated always as (x * 2) stored

);

SELECT a.attname,pg_catalog.format_type(a.atttypid, a.atttypmod),

  (SELECT pg_catalog.pg_get_expr(d.adbin, d.adrelid, true)

   FROM pg_catalog.pg_attrdef d

   WHERE d.adrelid = a.attrelid AND d.adnum = a.attnum AND a.atthasdef),

  a.attnotnull,

  (SELECT c.collname FROM pg_catalog.pg_collation c, pg_catalog.pg_type t

   WHERE c.oid = a.attcollation AND t.oid = a.atttypid AND
a.attcollation <> t.typcollation) AS attcollation,

  a.attidentity,

  a.attgenerated

FROM pg_catalog.pg_attribute a

WHERE a.attrelid = '74370' AND a.attnum > 0 AND NOT a.attisdropped

ORDER BY a.attnum;

attname | format_type |          pg_get_expr           | attnotnull |
attcollation | attidentity | attgenerated

--------+-------------+--------------------------------+------------+--------------+-------------+--------------

id      | integer     | nextval('k1_id_seq'::regclass) | t          |
-NULL-       |             |

x       | integer     | -NULL-                         | f          |
-NULL-       |             |

i_bd    | integer     | -NULL-                         | t          |
-NULL-       | d           |

i_a     | integer     | -NULL-                         | t          |
-NULL-       | a           |

i_as    | integer     | x * 2                          | f          |
-NULL-       |             | s

(5 rows) Now I just need to dig into pgtap and figure out what to do
with that. :) Kevin

[Jim Nasby]

On 8/25/21 9:24 AM, kevin wrote:

Now I just need to dig into pgtap and figure out what to do
with that.

Well, going back to my thoughts on what the use cases are...

1) Is the column GENERATED ... STORED or GENERATED ... AS IDENTITY? 

2) If STORED, what's the expression? 

3) If AS IDENTITY, is it ALWAYS or BY DEFAULT? 

4) If AS IDENTITY, what are the sequence options (if any)?

So spitballing some functions from that...

#1: identity_type_is(table, column, identity_type {STORED, GENERATED})

#2: identity_stored_expression_is(table, column, expression)

#3: identity_generated_when_is(table, column, when {ALWAYS, BY DEFAULT})

#4: identity_generated_sequence_options_are(table, column, sequence_options)

However, that's just a rough pass and there's definitely some options here...

a) Instead of treating STORED/IDENTITY and ALWAYS/BY DEFAULT as completely separate, we could combine them into something like {STORED, ALWAYS, BY DEFAULT}. That would reduce the number of functions. However I'm not much a fan of this idea since it means pgTap is diverging from how the engine actually works.

b) #1 is technically optional as long as the rest of the functions verify that the identity is of the expected type. IE, if you point identity_stored_expression_is() at a GENERATED ALWAYS identity, it needs to recognize that the type of identity is completely wrong. I don't think there's much precedent for doing this in pgTap though, so probably not a good idea.

c) Obviously I'm compromising English readability in order for all the function names to start with "identity_". Maybe that's not worth doing and the names should be things like "stored_identity_*" and "generated_identity_*"

David, what are your thoughts here?

[kevin]

On your thoughts above... I'm not sure some that makes sense to me, especially the first one. My reasoning is that IDENTITY isn't a type (like INT), it's an attribute (like NOT NULL) -- or at least the pg_* tables think so.

My tech lead was nice enough to let me work on this today. I'll toss my patch up and people can look to see what they think. Even if it's wholly rejected I'll still count it as a positive learning experience. :) I'm hoping it's close enough David can tweak and use it to at least cover the basics. We might want something more along the lines of what's above, but at least we'll have something that should hold us (where I work) while that's hashed out.

Kevin

[kevin]

On 8/25/21 4:42 PM, kevin wrote:
>
> My tech lead was nice enough to let me work on this today. I'll toss

> my patch up and people can look to see what they think...


Attached is a patch for review and test by whomever wants to do so. I think it does what's needed, or at least the col_default_is() function seemed to work for me. 😊 One of my goals was to change as little as possible, so there may be a better way to make this work if more extensive work is done.

I made an attempt at the doc by adding:

+The IDENTITY attribute is supported, just be aware that the value will be in
+lower case like:
+
+ SELECT col_default_is( 'tab', num1, 'generated always as identity' );
+ SELECT col_default_is( 'tab', num2, 'generated by default as identity' );
+ SELECT col_default_is( 'tab', num3, 'generated always as (x + y) stored' );

The point was to show that all 3 forms are supported. I did not attempt the "sequence_options"; I'll let someone else do that if they find it important.

I also made an attempt at changing test/sql/coltap.sql by adding 3 new columns and 3 new tests. I wasn't sure how to change expected/coltap.out since it looked like it was generated. Oh, I just realized that col_has_default() worked when I tried it 1 time, but I forgot a test for these there since I never use it so that needs to be added.

I also did not create a version patch in sql/ since I had no idea what the next version number would be ... 1.2.1 or 1.3.0 or what. Same for the Changes file.

David, You're free to take this and apply it with whatever changes you think are needed ... or toss it and do it a better way.

HTH,
Kevin

[Jim Nasby]

On 8/25/21 4:42 PM, kevin wrote:

> On your thoughts above... I'm not sure some that makes sense to me,
> especially the first one. My reasoning is that IDENTITY isn't a type
> (like INT), it's an attribute (like NOT NULL) -- or at least the pg_*
> tables think so.

The challenging bit is that both the behavior and the actual syntax
changes based on whether it's a stored identity or a generated one. That
makes trying to create a single API that handles both cases difficult;
especially when you consider that the community could add more options
in the future. This is a case where it's tough to strike a balance
between usability vs getting painted into a corner (and I'm definitely
not certain my proposal is the correct balance).

> My tech lead was nice enough to let me work on this today. I'll toss
> my patch up and people can look to see what they think. Even if it's
> wholly rejected I'll still count it as a positive learning experience.
> :) I'm hoping it's close enough David can tweak and use it to at least
> cover the basics. We might want something more along the lines of
> what's above, but at least we'll have something that should hold us
> (where I work) while that's hashed out.

Yes, thanks much! Just having the catalog stuff figured out is
definitely handy.

[David E. Wheeler]

On Aug 25, 2021, at 14:08, 'Jim Nasby' via pgTAP Users <pgtap...@googlegroups.com> wrote:
>
> #1: identity_type_is(table, column, identity_type {STORED, GENERATED})
>
> #2: identity_stored_expression_is(table, column, expression)
>
> #3: identity_generated_when_is(table, column, when {ALWAYS, BY DEFAULT})
>
> #4: identity_generated_sequence_options_are(table, column, sequence_options)
>
> However, that's just a rough pass and there's definitely some options here...
>
> a) Instead of treating STORED/IDENTITY and ALWAYS/BY DEFAULT as completely separate, we could combine them into something like {STORED, ALWAYS, BY DEFAULT}. That would reduce the number of functions. However I'm not much a fan of this idea since it means pgTap is diverging from how the engine actually works.
>
> b) #1 is technically optional as long as the rest of the functions verify that the identity is of the expected type. IE, if you point identity_stored_expression_is() at a GENERATED ALWAYS identity, it needs to recognize that the type of identity is completely wrong. I don't think there's much precedent for doing this in pgTap though, so probably not a good idea.
>
> c) Obviously I'm compromising English readability in order for all the function names to start with "identity_". Maybe that's not worth doing and the names should be things like "stored_identity_*" and "generated_identity_*"
>
> David, what are your thoughts here?

Not many, tbh. I wasn’t even aware of this feature before now. And just scanning the docs, it looks like syntax to implicitly create a sequence, like serial. But based on your description here, ti sounds more complicated than that.

On Aug 25, 2021, at 18:24, 'Jim Nasby' via pgTAP Users <pgtap...@googlegroups.com> wrote:

> The challenging bit is that both the behavior and the actual syntax changes based on whether it's a stored identity or a generated one. That makes trying to create a single API that handles both cases difficult; especially when you consider that the community could add more options in the future. This is a case where it's tough to strike a balance between usability vs getting painted into a corner (and I'm definitely not certain my proposal is the correct balance).

Your examples above remind me of the various tests for functions:

https://pgxn.org/dist/pgtap/doc/pgtap.html#Feeling.Funky

We have

• function_lang_is()
• function_returns()
• is_definer()
• isnt_definer()
• is_strict()
• isnt_strict()
• is_aggregate()
• isnt_aggregate()
• volatility_is()

So it makes sense to split them up, as you say, but perhaps less so if the community might add options in the future. Are the options likely to be stored in a descriptive way, like Kevin demonstrates in his patch? Adding a test for that kind of syntax ought to allow for new options in the future without requiring we change anything.

I assume this issue goes for all generated columns, not just generated identity columns, yes? Can it be generalized?

Best,

David

[kevin]

Well, it's 2'ish years later and I've moved onto another job, but I just
ran into this again. Looking at the current Pgtap docs I don't see any
functions to support this. And the syntax in the patch that I gave
doesn't work.  { SELECT col_default_is( 'table1', column2, 'generated by
default as identity' ); }

So... Is there way to do this now which I'm overlooking in the docs? Or
did the ball roll under the couch and get forgotten about? Or do I just
need to do a simple is() where the :have is a sub-select to get the
value of pg_attribute.attidentity for the column I need to test?

Please note, I'm not trying to point fingers and I'm not upset in any
way, I'm just trying to determine which path to take to make a test work
because I just got bit by a database not being correct and I didn't have
a test to tell me about the problem sooner.

Thanks!
Kevin