On Wed, Nov 6, 2024 at 10:26 AM Peter Smith <[email protected]> wrote:
>
> On Wed, Nov 6, 2024 at 3:26 PM Amit Kapila <[email protected]> wrote:
> >
> > On Wed, Nov 6, 2024 at 7:34 AM Peter Smith <[email protected]> wrote:
> > >
> > > Hi Vignesh,
> > >
> > > Here are my review comments for patch v49-0001.
> > >
> >
> > I have a question on the display of this new parameter.
> >
> > postgres=# \dRp+
> > Publication pub_gen
> > Owner | All tables | Inserts | Updates | Deletes | Truncates | Via
> > root | Generated columns
> > ----------+------------+---------+---------+---------+-----------+----------+-------------------
> > KapilaAm | f | t | t | t | t | f
> > | t
> > Tables:
> > "public.test_gen"
> >
> > The current theory for the display of the "Generated Columns" option
> > could be that let's add new parameters at the end which sounds
> > reasonable. The other way to look at it is how it would be easier for
> > users to interpret. I think the value of the "Via root" option should
> > be either after "All tables" or at the end as that is higher level
> > table information than operations or column-level information. As
> > currently, it is at the end, so "Generated Columns" should be added
> > before.
> >
> > Thoughts?
> >
>
> FWIW, I've always felt the CREATE PUBLICATION parameters
> publish
> publish_via_root
> publish_generated_columns
>
> Should be documented (e.g. on CREATE PUBLICATION page) in alphabetical order:
> publish
> publish_generated_columns
> publish_via_root
>
> ~
>
> Following on from that. IMO it will make sense for the describe
> (\dRp+) columns for those parameters to be in the same order as the
> parameters in the documentation. So the end result would be the same
> order as what you are wanting, even though the reason might be
> different.
>
Sounds reasonable to me.
I have made some minor comments and function name changes in the
attached. Please include in the next version.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/catalog/pg_publication.c
b/src/backend/catalog/pg_publication.c
index 92a5fa65e0..6dcb399ee3 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -261,15 +261,14 @@ is_schema_publication(Oid pubid)
* publication, false otherwise.
*
* If a column list is found, the corresponding bitmap is returned through the
- * cols parameter (if provided) and is constructed within the specified memory
- * context (mcxt).
+ * cols parameter, if provided. The bitmap is constructed within the given
+ * memory context (mcxt).
*/
bool
-check_fetch_column_list(Publication *pub, Oid relid, MemoryContext mcxt,
+check_and_fetch_column_list(Publication *pub, Oid relid, MemoryContext mcxt,
Bitmapset **cols)
{
- HeapTuple cftuple = NULL;
- Datum cfdatum = 0;
+ HeapTuple cftuple;
bool found = false;
if (pub->alltables)
@@ -280,6 +279,7 @@ check_fetch_column_list(Publication *pub, Oid relid,
MemoryContext mcxt,
ObjectIdGetDatum(pub->oid));
if (HeapTupleIsValid(cftuple))
{
+ Datum cfdatum;
bool isnull;
/* Lookup the column list attribute. */
@@ -289,7 +289,7 @@ check_fetch_column_list(Publication *pub, Oid relid,
MemoryContext mcxt,
/* Was a column list found? */
if (!isnull)
{
- /* Build the column list bitmap in the mcxt context. */
+ /* Build the column list bitmap in the given memory
context. */
if (cols)
*cols = pub_collist_to_bitmapset(*cols,
cfdatum, mcxt);
diff --git a/src/backend/replication/pgoutput/pgoutput.c
b/src/backend/replication/pgoutput/pgoutput.c
index 1a7b9dee10..32e1134b54 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1051,11 +1051,11 @@ check_and_init_gencol(PGOutputData *data, List
*publications,
foreach_ptr(Publication, pub, publications)
{
/*
- * The column list takes precedence over
'publish_generated_columns'
- * parameter. Those will be checked later, see
- * pgoutput_column_list_init.
+ * The column list takes precedence over the
+ * 'publish_generated_columns' parameter. Those will be checked
later,
+ * seepgoutput_column_list_init.
*/
- if (check_fetch_column_list(pub, entry->publish_as_relid, NULL,
NULL))
+ if (check_and_fetch_column_list(pub, entry->publish_as_relid,
NULL, NULL))
continue;
if (first)
@@ -1106,9 +1106,9 @@ pgoutput_column_list_init(PGOutputData *data, List
*publications,
Bitmapset *cols = NULL;
/* Retrieve the bitmap of columns for a column list
publication. */
- found_pub_collist |= check_fetch_column_list(pub,
-
entry->publish_as_relid,
-
entry->entry_cxt, &cols);
+ found_pub_collist |= check_and_fetch_column_list(pub,
+
entry->publish_as_relid,
+
entry->entry_cxt, &cols);
/*
* For non-column list publications — e.g. TABLE (without a
column
diff --git a/src/include/catalog/pg_publication.h
b/src/include/catalog/pg_publication.h
index c3302efd1a..71e5a5daf7 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -154,8 +154,8 @@ extern Oid GetTopMostAncestorInPublication(Oid puboid,
List *ancestors,
extern bool is_publishable_relation(Relation rel);
extern bool is_schema_publication(Oid pubid);
-extern bool check_fetch_column_list(Publication *pub, Oid relid,
-
MemoryContext mcxt, Bitmapset **cols);
+extern bool check_and_fetch_column_list(Publication *pub, Oid relid,
+
MemoryContext mcxt, Bitmapset **cols);
extern ObjectAddress publication_add_relation(Oid pubid, PublicationRelInfo
*pri,
bool if_not_exists);
extern Bitmapset *pub_collist_validate(Relation targetrel, List *columns);
