Re: Pgoutput not capturing the generated columns

Hi Vignesh,

Review comments for patch v53-0001:

I don't doubt that the latest 0001 patch is working OK, but IMO it
didn't need to be this complicated. I still think just using an alias
for all the columns not present as I suggested yesterday [1] would be
far simpler than the current patch.


On Sun, Jan 19, 2025 at 11:07 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Sun, 19 Jan 2025 at 06:39, Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi Vignesh,
> >
> > I was having some second thoughts about this patch and my previous suggestion.
> >
> > Currently the code is current written something like:
> >
> > printfPQExpBuffer(&buf,
> >  "SELECT oid, pubname,\n"
> >  "  pg_catalog.pg_get_userbyid(pubowner) AS owner,\n"
> >  "  puballtables, pubinsert, pubupdate, pubdelete");
> >
> > if (has_pubtruncate)
> >  appendPQExpBufferStr(&buf, ", pubtruncate");
> >
> > if (has_pubgencols)
> >  appendPQExpBufferStr(&buf, ", pubgencols");
> >
> > if (has_pubviaroot)
> >  appendPQExpBufferStr(&buf, ", pubviaroot");
> >
> > ~~
> >
> > IIUC the variable number of result columns (for different server
> > versions) is what is causing all the subsequent hassles.
> >
> > So, wouldn't the easiest fix be to change the code by adding the
> > appropriate 'else' alias for when the column is not available?
> >
> > Like this:
> >
> > printfPQExpBuffer(&buf,
> >  "SELECT oid, pubname,\n"
> >  "  pg_catalog.pg_get_userbyid(pubowner) AS owner,\n"
> >  "  puballtables, pubinsert, pubupdate, pubdelete");
> >
> > if (has_pubtruncate)
> >  appendPQExpBufferStr(&buf, ", pubtruncate");
> > else
> >  appendPQExpBufferStr(&buf, ", 'f' AS pubtruncate");
> >
> > if (has_pubgencols)
> >  appendPQExpBufferStr(&buf, ", pubgencols");
> > else
> >  appendPQExpBufferStr(&buf, ", 'f' AS pubgencols");
> >
> > if (has_pubviaroot)
> >  appendPQExpBufferStr(&buf, ", pubviaroot");
> > else
> >  appendPQExpBufferStr(&buf, ", 'f' AS pubviaroot");
> >
> > ~~
> >
> > Unless I am mistaken this will simplify the subsequent code a lot because:
> > 1. Now you can put the cols in the same order you want to display them
> > 2. Now the tuple result has a fixed number of cols for all server versions
> > 3. Now hardcoding the indexes (1,2,3,4...) is fine because they are
> > always the same
> >
> > Thoughts?
>
> We typically use this approach when performing a dump, where we
> retrieve the default values for older versions and store them in a
> structure. The values are then included in the dump only if they
> differ from the default. However, this approach cannot be used with
> psql because older version servers may not have these columns. As a
> result, we must avoid displaying these columns when interacting with
> older version servers.
>

Maybe I have some fundamental misunderstanding here, but I don't see
why "this approach cannot be used with psql because older version
servers may not have these columns". Not having the columns is the
whole point of using an alias approach in the first place e.g. the
below table t1 does not have a column called "banana" but it works
just fine to select an alias using that name...

test_pub=# CREATE TABLE t1 (a int, b int);
CREATE TABLE

test_pub=# INSERT INTO t1 VALUES (123,456);
INSERT 0 1

test_pub=# SELECT a, b, 999 AS banana FROM t1;
  a  |  b  | banana
-----+-----+--------
 123 | 456 |    999
(1 row)


And you also said: "we must avoid displaying these columns when
interacting with older version servers".

But, why must we? I don't know why it is a bad thing to display
"Generated columns" as 'f' for a PG17 server. Anyway, you are always
at liberty to not display the missing columns if that is what you
want... just call the 'printTableAddCell' conditionally the same as
now. Except now, the PQgetvalue can have just a simple fixed index
because the SELECT (with aliases) will also have a fixed number of
columns in the result.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPs3ORjQTcce0MyHyT3aEqCh_jyUMNHUjm0XcrxKNEM8-Q%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia