identity work around [was: pgtap identity error]

[kevin]

One more on this and I'm done for the time being ... unless someone asks questions about it. Here's my work around so others can find it on a search, as well as some notes on how I'd approach this if I were modifying pgTap.

When I do testing, I control it with a bash script. In that script I do:
    psql -X -d $PGDATABASE < get_col_ident.sql

That file looks like:
-- a function for testing since pgtap doesn't have an equivalent of this
-- remove when pgtap can check for column identities
CREATE OR REPLACE FUNCTION public._get_column_identity(
    p_schema    VARCHAR,
    p_table     VARCHAR,
    p_column    VARCHAR
    )
    RETURNS CHAR AS $$
        SELECT coalesce(a.attidentity, a.attgeneratd)
        FROM pg_catalog.pg_attribute a
        WHERE a.attrelid =
            ( SELECT c.oid FROM pg_catalog.pg_class c LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace
              WHERE n.nspname = p_schema AND c.relname = p_table )
          AND a.attnum > 0
          AND NOT a.attisdropped
          AND a.attname = p_column
          AND pg_catalog.format_type(a.atttypid, a.atttypmod) in ( 'integer', 'smallinteger', 'biginteger' )
          AND a.attnotnull ;
    $$ LANGUAGE SQL;

So that hides some ugly sql. Once I have that, then in my testing files I can do things like:

select is( (select _get_column_identity('schema','table','column')), 'd', 'column has default identity');
-- or
select is( (select _get_column_identity('schema','table','column')), 'a', 'column has always identity');
-- or
select is( (select _get_column_identity('schema','table','column')), 's', 'column has stored identity');

That has solved the basic problem for me and allows me to go on ... and find & fix problems.


Now, that is kind of ugly, so what would I do if I was changing pgTap? Taking some of Jim's and David's thoughts on it and was as what I've learned, I'd create:

col_identity_is(:schema, :table, :column, :type [, :desc]);
col_identity_options_are(:schema, :table, :column, :options, [, :desc])

where :type is one of: 'always', 'default', 'stored'
where :options is one of an "expression" or "sequence options"  (yes, I'm combining 2 tests into 1)

I think that fits into pgTap's style, although if someone wanted to split that last one into:

col_identity_options_are(:schema, :table, :column, ARRAY[ :options ], [, :desc])  -- this might be required the more i think on it
col_identity_expression_is(:schema, :table, :column, :expression, [, :desc])

I wouldn't argue as I can see that, but those 2 are only needed when the identity is "stored". If anyone would like (just ask) I can give you pseudo code for how to write the two functions. I've outlined them that way, but I haven't bothered converting the pseudo code into real PLPGSQL because the example at the top works well enough for me at the moment, I don't use the stored option at all, and I'm a bit busy doing other work at the moment.

HTH,
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 to go play with the dust bunnies 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


On 8/29/21 18:31, David E. Wheeler wrote:
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

[David E. Wheeler]

Hi Kevin, apologies for the delayed reply. Super appreciate you following up here.

On Sep 12, 2023, at 15:02, kevin <kbra...@pwhome.com> wrote:

> I wouldn't argue as I can see that, but those 2 are only needed when the identity is "stored". If anyone would like (just ask) I can give you pseudo code for how to write the two functions. I've outlined them that way, but I haven't bothered converting the pseudo code into real PLPGSQL because the example at the top works well enough for me at the moment, I don't use the stored option at all, and I'm a bit busy doing other work at the moment.

This is great, thank you. I personally am not familiar with this feature and haven’t had the bandwidth to dig into it, but I opened issue 321 (https://github.com/theory/pgtap/issues/321) to point to this thread so that it doesn’t get lost in the cracks again. Please feel free to add additional content to that issue.

Would you be interested in trying to patch pgTAP itself with these features?

Best,

David

[kevin]

David,

Maybe. :) I'm willing to contribute, but it really does depend on how
much time I have. My previous try to change pgTap's code was an
adventure. :)

Thanks for creating the bug/issue. I would have done that but for some
reason it just escaped me to do so. I'll go add my notes to it.

I think we should ignore the previous patch I submitted (using
col_default_is) as I can now see that's really the wrong way to go.
(Sometimes I have to throw the mud at the wall a few times before I can
figure out what really sticks.) I think the path to take is in my last
email with:

    col_identity_is(:schema, :table, :column, :type [, :desc]);

    col_identity_options_are(:schema, :table, :column, ARRAY[ :options
], [, :desc])

    col_identity_expression_is(:schema, :table, :column, :expression,
[, :desc])

At present, I think I'll split the work into 2 parts. Part 1 is the
first function as that's the most bang for the buck in many ways. Part 2
will be the last 2 functions since they won't be used as much. I'll do
my best best to keep this somewhere on my radar and not forget about it.

Kevin

On 9/24/23 19:04, David E. Wheeler wrote:
> Hi Kevin, apologies for the delayed reply. Super appreciate you following up here.
>
> On Sep 12, 2023, at 15:02, kevin <kbra...@pwhome.com> wrote:
>

>> I wouldn't argue as I can see that, but those 2 are only needed when the identity is "stored". If anyone would like (just ask) I can give you pseudo code for how to write the two functions. I've outlined them that way, but I haven't bothered converting the pseudo code into real PLPGSQL because the example at the top works well enough for me at the moment, I don't use the stored option at all, and I'm a bit busy doing other work at the moment.

[David E. Wheeler]

On Sep 25, 2023, at 11:50, kevin <kbra...@pwhome.com> wrote:

> I think we should ignore the previous patch I submitted (using col_default_is) as I can now see that's really the wrong way to go. (Sometimes I have to throw the mud at the wall a few times before I can figure out what really sticks.)

Hey, that’s just good engineering practice. :-)

> I think the path to take is in my last email with:
> col_identity_is(:schema, :table, :column, :type [, :desc]);
> col_identity_options_are(:schema, :table, :column, ARRAY[ :options ], [, :desc])
> col_identity_expression_is(:schema, :table, :column, :expression, [, :desc])
>
> At present, I think I'll split the work into 2 parts. Part 1 is the first function as that's the most bang for the buck in many ways. Part 2 will be the last 2 functions since they won't be used as much. I'll do my best best to keep this somewhere on my radar and not forget about it.

Sounds good. Maybe it should be `col_identity_type_is()`, though? I’m just bike-shedding over here, TBH.

D