Date: Thu, 19 May 2022 07:07:24 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, houzj.fnst@fujitsu.com,
	Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: bogus: logical replication rows/cols combinations
Message-ID: <20220519120724.GO19626@telsasoft.com>
References: 
 <CAA4eK1+UCfKUaf41H6zHS2qR4CZoVtZe64yx_T-JVPSb1pdw_w@mail.gmail.com>
 <202205161320.butjtubabvgj@alvherre.pgsql>
 <CAA4eK1KfL=ez5fKPB-0Nrgf7wiqN9bXP-YHHj2YH5utXAmjYug@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAA4eK1KfL=ez5fKPB-0Nrgf7wiqN9bXP-YHHj2YH5utXAmjYug@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220519120724.GO19626%40telsasoft.com>
Precedence: bulk

On Thu, May 19, 2022 at 10:33:13AM +0530, Amit Kapila wrote:
> I have committed the first patch after fixing this part. It seems Tom
> is not very happy doing this after beta-1 [1]. The reason we get this
> information via this view (and underlying function) is that it
> simplifies the queries on the subscriber-side as you can see in the
> second patch. The query change is as below:
> [1] - https://www.postgresql.org/message-id/91075.1652929852%40sss.pgh.pa.us

I think Tom's concern is that adding information to a view seems like adding a
feature that hadn't previously been contemplated.
(Catalog changes themselves are not prohibited during the beta period).

> a. Revert the change in view (and underlying function) as done in
> commit 0ff20288e1 and consider the alternate way (using a slightly
> complex query) to fix. Then maybe for PG-16, we can simplify it by
> changing the underlying function and view.

But, ISTM that it makes no sense to do it differently for v15 just to avoid the
appearance of adding a new feature, only to re-do it in 2 weeks for v16...
So (from a passive observer) +0.1 to keep the current patch.

I have some minor language fixes to that patch.

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index d96c72e5310..82aa84e96e1 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9691,7 +9691,7 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
 
      <row>
       <entry><link linkend="view-pg-publication-tables"><structname>pg_publication_tables</structname></link></entry>
-      <entry>publications and information of their associated tables</entry>
+      <entry>publications and information about their associated tables</entry>
      </row>
 
      <row>
@@ -11635,7 +11635,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
 
   <para>
    The view <structname>pg_publication_tables</structname> provides
-   information about the mapping between publications and information of
+   information about the mapping between publications and information about
    tables they contain.  Unlike the underlying catalog
    <link linkend="catalog-pg-publication-rel"><structname>pg_publication_rel</structname></link>,
    this view expands publications defined as <literal>FOR ALL TABLES</literal>
@@ -11695,7 +11695,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
       </para>
       <para>
        Names of table columns included in the publication. This contains all
-       the columns of the table when the user didn't specify the column list
+       the columns of the table when the user didn't specify a column list
        for the table.
       </para></entry>
      </row>
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 8c7fca62de3..2f706f638ce 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -1077,7 +1077,7 @@ get_publication_name(Oid pubid, bool missing_ok)
 }
 
 /*
- * Returns information of tables in a publication.
+ * Returns information about tables in a publication.
  */
 Datum
 pg_get_publication_tables(PG_FUNCTION_ARGS)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 87aa571a331..86f13293090 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -11673,7 +11673,7 @@
   prosrc => 'pg_show_replication_origin_status' },
 
 # publications
-{ oid => '6119', descr => 'get information of tables in a publication',
+{ oid => '6119', descr => 'get information about tables in a publication',
   proname => 'pg_get_publication_tables', prorows => '1000', proretset => 't',
   provolatile => 's', prorettype => 'record', proargtypes => 'text',
   proallargtypes => '{text,oid,int2vector,pg_node_tree}', proargmodes => '{i,o,o,o}',