Re: Add pg_get_publication_ddl function - Mailing list pgsql-hackers

Hi Jonathan,

Here are some initial review comments for patch v1-0001.

(I haven't applied or run anything -- this is all just by inspection).

======
1. GENERAL - typos

There are many typos and upper/lower capitalisation mistakes in the
comments / commit message/docs.

I won't list them all, but below is a sample. Consider using a
spelling/grammar checker to discover all of them.

typo /for a giving PUBLICATION./
typo /thuse/
typo /Comprhensive/
typo /CREATE statement for a giving PUBLICATION/
typo /DDL fro a PUBLICATION/
typo /The sequence are/
typo /publication an have relations/
typo /If no null/
typo /at the end make sense to/
typo /create publation/

======

doc/src/sgml/func/func-info.sgml

2.
+     <tbody>
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>pg_get_publication_ddl</primary>
+        </indexterm>
+        <function>pg_get_publication_ddl</function> (
<parameter>publication</parameter> <type>text</type> or
<type>oid</type> )
+        <returnvalue>text</returnvalue>
+       </para>

Maybe say "name" instead of "text" for the parameter; otherwise, there
is no indication of what the text should represent.

~~~

3.
+       <para>
+        Recreate the CREATE statement for a giving PUBLICATION.
+        The result is a complete <command>CREATE
PUBLICATION</command> statement.
+       </para></entry>
+      </row>
+     </tbody>

SUGGESTION
Returns the <command>CREATE PUBLICATION</command> command that would
create this publication.

======
src/backend/utils/adt/ruleutils.c

pg_do_publication_ddl:

4.
+ if (pub == NULL)
+ return (Datum) NULL;

Was a cast needed?

~~~

5.
+ publications = GetPublicationRelations(pub->oid, PUBLICATION_PART_ALL);

AFAIK, this returns relids of the published tables. So this variable
name 'publications' is very misleading.

~~~

6.
+ /*
+ * we add the publication name, this isn't covered by the schema, yet?
+ */

What does that "covered by the schema" part mean? Pubnames are not
schema-qualified (??)

~~~

7.
+ /*
+ * having all tables or all sequence means there's not publications per
+ * table
+ */

This might be true for now, but it might be wise to avoid making this
assumption if it is possible to do so, because one day, "FOR ALL
SEQUENCES, FOR TABLE t1" syntax may be added, and then your assumption
will be broken.

IOW, would it be better not to say 'else'?

~~~

8.
+ if (pub->allsequences)
+ appendStringInfo(buf,
+ "%sALL SEQUENCE",
+ pub->alltables ? ", " : " ");

This should say "ALL SEQUENCES", not "ALL SEQUENCE"

~~~

9.
+ /*
+ * publication an have relations of tables, in schema or current
+ * schema
+ *
+ */

9a.
Unnecessary newline.

~

9b.
Why are you mentioning FOR TABLES IN SCHEMA here? That seems
unnecessary. That clause gets handled separately later.

~~~~

10.
+ /* If no null, means we have a list of columns to publish */
+ if (!cols_nulls)
+ {
+ ArrayType  *arr;
+ int ncolumns;
+ int16    *cols;
+ bool fcol = true;
+
+ arr = DatumGetArrayTypeP(columns);
+ ncolumns = ARR_DIMS(arr)[0];
+ cols = (int16 *) ARR_DATA_PTR(arr);
+
+ appendStringInfoChar(buf, '(');
+ for (int i = 0; i < ncolumns; i++)
+ {
+ appendStringInfo(buf, "%s%s",
+ fcol ? "" : ", ",
+ get_attname(rel->rd_id, cols[i], true));
+ fcol = false;
+ }
+ appendStringInfoChar(buf, ')');
+ }

10a.
SUGGESTION
Handle any Column List for this table

~

10b.
Variable 'fcol' is unnecessary; just check i == 0.

~~~

11.
+ /*
+ * If there's a condition goes after the columns but if there's no
+ * column we can have conditions too
+ */

For everything related to Anum_pg_publication_rel_prattrs and
Anum_pg_publication_rel_prqual, the well-known terminology is "Column
Lists" and "Row Filters", so all the variables and comments using
those should be calling them by the proper names.

IIUC, Row Filters are independent of Column Lists, so that code
comment is not very helpful.

SUGGESTION
Handle any Row Filter for this table

~~~

12.
+ appendStringInfo(buf, " WHERE %s ", str);

Why the trailing space? You didn't have one if there was only a Column
List, but no Row Filter.

~~~

13.
+ /* if we have schemas, for sure this will go right before the WITH */

Overcomplicated comment, you don't need to say stuff like "for sure
this will go right before the WITH" because the DDL order of clauses
is obvious from the logic.

SUGGESTION
Handle FOR TABLES IN SCHEMA

~~~

14.
+
+
+ /* always add the WITH options */
+ appendStringInfo(buf, " WITH (");

Double blank line.

~~~

15.
+ /* publish string */
+ appendStringInfo(buf, "publish='");

SUGGESTION
Parameter: 'publish'

~~~

16.
+ /*
+ * by precedence we know that the insert will always be first, no need
+ * to check previous values
+ */

There is no "precedence". The 'publish' values are in this order only
because you chose to put them in this order. This whole comment seems
unnecessary.

~~~

17.
+ /* publish_generated_columns string */
+ appendStringInfo(buf, "publish_generated_columns='%s', ",
+ pub->pubgencols_type == PUBLISH_GENCOLS_NONE ? "none" : "stored");

SUGGESTION
Parameter: 'publish_generated_columns'

~~~

18.
+ /* publish_via_partition_root value */
+ appendStringInfo(buf, "publish_via_partition_root='%s')",
+ pub->pubviaroot ? "true" : "false");

18a
SUGGESTION
Parameter: 'publish_via_partition_root'

~

18b.
Putting the true/false in single quotes is unnecessary, and it looks a
bit strange

======
src/test/regress/sql/publication_ddl.sql

19. GENERAL

I have not checked the tests in detail, but here are some general comments:

19a. There is a mix of upper/lowercase -- e/g/sometimes you say
"create publication", other times you say "ALTER PUBLICATION". Be
consistent.

~

19b.
Some tests are missing comments to introduce them. The spacing
grouping the tests is also inconsistent.

~

19c.
Use proper terminology of "Column Lists" and "Row Filters" instead of
just calling them columns and conditions

~

19d.
All of those cleanup DROPs can be combined; You don't need a separate
DROP statement for every object.

~~~

20.
+
+-- a new schema for multiple schemas
+CREATE SCHEMA pub_schema_test_ddl_2;
+CREATE TABLE pub_schema_test_ddl_2.schema_tbl1 (foo int, bar int);

This could've been done earlier same place where you created the other
test schema/tables.

======

FYI, last year I wrote a blog [1] claiming to be the  "complete syntax
guide" for CREATE PUBLICATION. Perhaps it is useful as a reference?

======
[1]
https://www.postgresql.fastware.com/pzone/tailoring-create-publication-for-logical-replication-a-complete-syntax-guide

Kind Regards,
Peter Smith.
Fujitsu Australia