Re: Logical Replication of sequences

On Mon, 6 Jan 2025 at 10:46, Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.

Modified

> ======
> 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?

Modified

> ======
> 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.

Modified

> ======
> 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

I feel it is not required

> ~~~
>
> 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?

Modified

> ~~~
>
> 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?

Modified

The attached v20250110 version patch has the changes for the same.

Regards,
Vignesh