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

Hi Vignesh,

Here are some review comments for the patch v20241211-0002.

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

1.
+<phrase>where <replaceable class="parameter">object
type</replaceable> is one of:</phrase>
+
+    TABLES
+    SEQUENCES

The replaceable "object_type" is missing an underscore.

~~~

publish option effect fro SEQUENCE replication?

2.
It's not obvious to me if the SEQUENCE replication stuff is affected
but the setting of pubactions (ie the 'publish' option). I'm thinking
that probably anything to do with SEQUENCEs may no be considered a DML
operation, but if that is true it might be better to explicitly say
so.

Also, we might need to include a test to show even if publish='' that
the SEQUENCE synchronization is not affected.


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

CreatePublication:

3.
- /* 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 a %s publication",
+ stmt->for_all_tables ? "FOR ALL TABLES" :
+ "FOR ALL SEQUENCES")));

It seems a quirk that if FOR ALL TABLES and FOR ALL SEQUENCES are
specified at the same time then you would only get the "FOR ALL
TABLES" error, but maybe that is OK?

~~~

AlterPublicationOwner_internal:

4.
Ditto quirk as commented above, but maybe it is OK.

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

describePublications:

5.
It seems the ordering of the local variables, and then the attributes
in the SQL, and the headings in the "describe" output are a bit
muddled.

IMO it might be better to always keep things in the same order as the
eventual display headings. So anything to do with puballsequences
should be immediately after anything to do with puballtables.

(There are multiple changes needed in this function to rearrange
things to be this way).

~~~

6.
The following seems wrong because now somehow there are two lots of
index 9 (???)

-----
printTableAddCell(&cont, PQgetvalue(res, i, 2), false, false);
printTableAddCell(&cont, PQgetvalue(res, i, 3), false, false);
if (has_pubsequence)
  printTableAddCell(&cont, PQgetvalue(res, i, 9), false, false); /*
all sequences */
printTableAddCell(&cont, PQgetvalue(res, i, 4), false, false);
printTableAddCell(&cont, PQgetvalue(res, i, 5), false, false);
printTableAddCell(&cont, PQgetvalue(res, i, 6), false, false);
if (has_pubtruncate)
  printTableAddCell(&cont, PQgetvalue(res, i, 7), false, false);
if (has_pubgencols)
  printTableAddCell(&cont, PQgetvalue(res, i, 8), false, false);
if (has_pubviaroot)
  printTableAddCell(&cont, PQgetvalue(res, i, 9), false, false);
-----


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

7.
+\dRp+ regress_pub_forallsequences1
+                                            Publication
regress_pub_forallsequences1
+          Owner           | All tables | All sequences | Inserts |
Updates | Deletes | Truncates | Generated columns | Via root

+--------------------------+------------+---------------+---------+---------+---------+-----------+-------------------+----------
+ regress_publication_user | f          | f             | t       | t
     | t       | t         | f                 | f
+(1 row)
+

The expected value of 'f' for "All sequences" looks wrong to me. I
think this might be a manifestation of that duplicated '9' index
mentioned in an earlier review comment #6.

~~~

8.
+\dRp+ regress_pub_for_allsequences_alltables
+                                       Publication
regress_pub_for_allsequences_alltables
+          Owner           | All tables | All sequences | Inserts |
Updates | Deletes | Truncates | Generated columns | Via root

+--------------------------+------------+---------------+---------+---------+---------+-----------+-------------------+----------
+ regress_publication_user | t          | f             | t       | t
     | t       | t         | f                 | f
+(1 row)
+

The expected value of 'f' for "All sequences" looks wrong to me. I
think this might be a manifestation of that duplicated '9' index
mentioned in an earlier review comment #6.

~~~

9.
 \dRp+ testpub_forparted
-                                         Publication testpub_forparted
-          Owner           | All tables | Inserts | Updates | Deletes
| Truncates | Generated columns | Via root
---------------------------+------------+---------+---------+---------+-----------+-------------------+----------
- regress_publication_user | f          | t       | t       | t
| t         | f                 | t
+                                                 Publication testpub_forparted
+          Owner           | All tables | All sequences | Inserts |
Updates | Deletes | Truncates | Generated columns | Via root

+--------------------------+------------+---------------+---------+---------+---------+-----------+-------------------+----------
+ regress_publication_user | f          | t             | t       | t
     | t       | t         | f                 | t


AFAIK these partition tests should not be impacting the "All
Sequences" flag value, so the expected value of 't' for "All
sequences" looks wrong to me. I think this might be a manifestation of
that duplicated '9' index mentioned in an earlier review comment #6.

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