Re: Logical Replication of sequences - Mailing list pgsql-hackers

Hi Vignesh,

FYI, looks like your attached patchset was misnamed 20250204 instead
of 20250104. Anyway, it does not affect the reviews, but I am going to
refer to it as 0104 from now.

~~~

I have no comments for the patch v20250104-0001.

Some comments for the patch v20250104-0002.

======
doc/src/sgml/ref/create_publication.sgml

1.
 <phrase>where <replaceable
class="parameter">publication_object</replaceable> is one of:</phrase>

+    ALL TABLES
+    ALL SEQUENCES
     TABLE [ ONLY ] <replaceable
class="parameter">table_name</replaceable> [ * ] [ ( <replaceable
class="parameter">column_name</replaceable> [, ... ] ) ] [ WHERE (
<replaceable class="parameter">expression</replaceable> ) ] [, ... ]
     TABLES IN SCHEMA { <replaceable
class="parameter">schema_name</replaceable> | CURRENT_SCHEMA } [, ...
]

I'm wondering if it would be better to reorder these in the synopsis as:
TABLE...
TABLES IN SCHEMA...
ALL TABLES
ALL SEQUENCES

Then it will match the same order as the parameters section.

======
src/backend/commands/publicationcmds.c

2.
> > CreatePublication:
> >
> > 2.
> > - /* FOR ALL TABLES requires superuser */
> > - if (stmt->for_all_tables && !superuser())
> > + /* FOR ALL TABLES or FOR ALL SEQUENCES requires superuser */
> > + if ((stmt->for_all_tables || stmt->for_all_sequences) && !superuser())
> >   ereport(ERROR,
> >   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> > - errmsg("must be superuser to create FOR ALL TABLES publication")));
> > + errmsg("must be superuser to create ALL TABLES and/or ALL SEQUENCES
> > publication")));
> >
> >
> > 2b.
> > This message might be OK now, but I suspect it will become very messy
> > in future after you introduce another syntax like "FOR SEQUENCE
> > seq_name" etc (which would also be able to be used in combination with
> > a FOR ALL TABLES).
> >
> > So, I think that for future-proofing against all the possible (future)
> > combinations, and for keeping the code cleaner, it will be far simpler
> > to just keep the errors for tables and sequences separated:
> >
> > SUGGESTION:
> > if (!superuser())
> > {
> >   if (stmt->for_all_tables)
> >     ereport(ERROR, ... FOR ALL TABLES ...);
> >   if (stmt->for_all_sequences)
> >     ereport(ERROR, ... FOR ALL SEQUENCES ...);
> > }
>
> If we do that way it will not print both the stmt publication type if
> both "ALL TABLES" and "ALL SEQUENCES" is specified.
>

Yes, I know, but AFAICT you're going to encounter this same kind of
problem anyway with all the other combinations, where we only give an
error for the first thing it finds wrong.

For example,
CREATE FOR ALL SEQUENCES, TABLES IN SCHEMA s1;

That's going to report "must be superuser to create a FOR ALL TABLES
and/or a FOR ALL SEQUENCES publication", but it's not going to say
"must be superuser to create FOR TABLES IN SCHEMA publication".

So, my point was, I guess we are not going to make error messages for
every possible combination, so why are we making a special case by
combining only the message for ALL TABLES and ALL SEQUENCES?

======
src/bin/psql/describe.c

3.
+ if (has_pubsequence)
+ printTableAddCell(&cont, PQgetvalue(res, i, puballsequences_col),
false, false); /* all sequences */

The comment ("/* all sequences */") doesn't seem necessary given the
self-explanatory variable name. Also, none of the similar nearby code
has comments like this.

======
src/test/regress/expected/publication.out

4.
+--- Specifying both ALL TABLES and ALL SEQUENCES
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION regress_pub_for_allsequences_alltables FOR ALL
SEQUENCES, ALL TABLES;


Do you think you should test this both ways?
e.g.1. FOR ALL SEQUENCES, ALL TABLES
e.g.2. FOR ALL TABLES, ALL SEQUENCES

~~~

5.
+DROP PUBLICATION regress_pub_forallsequences1,
regress_pub_forallsequences2, regress_pub_for_allsequences_alltables;
+-- fail - Specifying ALL TABLES more than once
+CREATE PUBLICATION regress_pub_for_allsequences_alltables FOR ALL
SEQUENCES, ALL TABLES, ALL TABLES;
+ERROR:  invalid publication object list
+LINE 1: ...equences_alltables FOR ALL SEQUENCES, ALL TABLES, ALL TABLES...
+                                                             ^
+DETAIL:  TABLES can be specified only once.

Should the DETAIL message say ALL TABLES instead of just TABLES?

~~~

6.
+-- fail - Specifying ALL SEQUENCES more than once
+CREATE PUBLICATION regress_pub_for_allsequences_alltables FOR ALL
SEQUENCES, ALL TABLES, ALL SEQUENCES;
+ERROR:  invalid publication object list
+LINE 1: ...equences_alltables FOR ALL SEQUENCES, ALL TABLES, ALL SEQUEN...
+                                                             ^
+DETAIL:  SEQUENCES can be specified only once.

Should the DETAIL message say ALL SEQUENCES instead of just SEQUENCES?

======
Kind Regards,
Peter Smith.
Fujitsu Australia