Some review comments for patch v54-0004.
(since preparing this post, I saw you have already posted v55-0001 but
AFAIK, since 55-0001 is just a merge, the same review comments are
still applicable)
======
doc/src/sgml/catalogs.sgml
1.
<para>
- If true, this publication replicates the stored generated columns
- present in the tables associated with the publication.
+ <literal>n</literal> indicates that the generated columns in the tables
+ associated with the publication should not be replicated.
+ <literal>s</literal> indicates that the stored generated columns in the
+ tables associated with the publication should be replicated.
</para></entry>
It looks OK, but maybe we should use a wording style similar to that
used already for pg_subscription.substream?
Also, should this mention column lists?
SUGGESTION
Indicates how to handle generated column replication (when there is no
publication column list): <literal>n</literal> = generated columns in
the tables associated with the publication should not be replicated;
<literal>s</literal> = stored generated columns in the tables
associated with the publication should be replicated.
======
src/backend/commands/publicationcmds.c
parse_publication_options:
2.
bool *publish_generated_columns_given,
- bool *publish_generated_columns)
+ char *publish_generated_columns)
Why not use the PublishGencolsType enum here?
~~~
CreatePublication:
3.
- bool publish_generated_columns;
+ char publish_generated_columns;
AclResult aclresult;
Why not use the PublishGencolsType enum here?
~~~
AlterPublicationOptions:
4.
bool publish_generated_columns_given;
- bool publish_generated_columns;
+ char publish_generated_columns;
Why not use the PublishGencolsType enum here?
~~~
defGetGeneratedColsOption::
5.
+/*
+ * Extract the publish_generated_columns option value from a DefElem. "stored"
+ * and "none" values are accepted.
+ */
+static char
+defGetGeneratedColsOption(DefElem *def)
+{
The return type should be PublishGencolsType.
======
src/include/catalog/pg_publication.h
6.
- /* true if generated columns data should be published */
- bool pubgencols;
+ /*
+ * none if generated column data should not be published. stored if stored
+ * generated column data should be published.
+ */
+ char pubgencols_type;
} FormData_pg_publication;
Maybe this was accidentally changed by some global replacement you
did. IMO the previous (v53) version comment was better here.
- bool pubgencols;
+ /*
+ * 'n'(none) if generated column data should not be published.
+ * 's'(stored) if stored generated column data should be published.
+ */
======
FYI, please see the attached diffs where I have already made some of
these changes while experimenting with the suggestions.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e39612f..9391922 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6399,10 +6399,12 @@ SCRAM-SHA-256$<replaceable><iteration
count></replaceable>:<replaceable>&l
<structfield>pubgencols</structfield> <type>char</type>
</para>
<para>
- <literal>n</literal> indicates that the generated columns in the tables
- associated with the publication should not be replicated.
- <literal>s</literal> indicates that the stored generated columns in the
- tables associated with the publication should be replicated.
+ Indicates how to handle generated column replication when there is no
+ publication column list:
+ <literal>n</literal> = generated columns in the tables associated with
+ the publication should not be replicated;
+ <literal>s</literal> = stored generated columns in the tables associated
+ with the publication should be replicated.
</para></entry>
</row>
diff --git a/src/backend/commands/publicationcmds.c
b/src/backend/commands/publicationcmds.c
index b49d9ab..1c51356 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -70,7 +70,7 @@ static void PublicationDropTables(Oid pubid, List *rels, bool
missing_ok);
static void PublicationAddSchemas(Oid pubid, List *schemas, bool if_not_exists,
AlterPublicationStmt *stmt);
static void PublicationDropSchemas(Oid pubid, List *schemas, bool missing_ok);
-static char defGetGeneratedColsOption(DefElem *def);
+static PublishGencolsType defGetGeneratedColsOption(DefElem *def);
static void
@@ -81,7 +81,7 @@ parse_publication_options(ParseState *pstate,
bool
*publish_via_partition_root_given,
bool
*publish_via_partition_root,
bool
*publish_generated_columns_given,
- char
*publish_generated_columns)
+ PublishGencolsType
*publish_generated_columns)
{
ListCell *lc;
@@ -777,7 +777,7 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt
*stmt)
bool publish_via_partition_root_given;
bool publish_via_partition_root;
bool publish_generated_columns_given;
- char publish_generated_columns;
+ PublishGencolsType publish_generated_columns;
AclResult aclresult;
List *relations = NIL;
List *schemaidlist = NIL;
@@ -837,7 +837,7 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt
*stmt)
values[Anum_pg_publication_pubviaroot - 1] =
BoolGetDatum(publish_via_partition_root);
values[Anum_pg_publication_pubgencols_type - 1] =
- CharGetDatum(publish_generated_columns);
+ CharGetDatum((char)publish_generated_columns);
tup = heap_form_tuple(RelationGetDescr(rel), values, nulls);
@@ -924,7 +924,7 @@ AlterPublicationOptions(ParseState *pstate,
AlterPublicationStmt *stmt,
bool publish_via_partition_root_given;
bool publish_via_partition_root;
bool publish_generated_columns_given;
- char publish_generated_columns;
+ PublishGencolsType publish_generated_columns;
ObjectAddress obj;
Form_pg_publication pubform;
List *root_relids = NIL;
@@ -1048,7 +1048,7 @@ AlterPublicationOptions(ParseState *pstate,
AlterPublicationStmt *stmt,
if (publish_generated_columns_given)
{
- values[Anum_pg_publication_pubgencols_type - 1] =
CharGetDatum(publish_generated_columns);
+ values[Anum_pg_publication_pubgencols_type - 1] =
CharGetDatum((char)publish_generated_columns);
replaces[Anum_pg_publication_pubgencols_type - 1] = true;
}
@@ -2050,7 +2050,7 @@ AlterPublicationOwner_oid(Oid subid, Oid newOwnerId)
* Extract the publish_generated_columns option value from a DefElem. "stored"
* and "none" values are accepted.
*/
-static char
+static PublishGencolsType
defGetGeneratedColsOption(DefElem *def)
{
char *sval;
