Re: bogus: logical replication rows/cols combinations

On Thu, May 19, 2022 at 7:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > 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).
>
> It certainly smells like a new feature, but my concern was more around the
> post-beta catalog change.  We do those only if really forced to, and the
> explanation in the commit message didn't satisfy me as to why it was
> necessary.  This explanation isn't much better --- if we're trying to
> prohibit a certain class of publication definitions, what good does it do
> to check that on the subscriber side?
>

It is required on the subscriber side because prohibition is only for
the cases where multiple publications are combined. We disallow the
cases where the column list is different for the same table when
combining publications. For example:

Publisher-side:
Create table tab(c1 int, c2 int);
Create Publication pub1 for table tab(c1);
Create Publication pub1 for table tab(c2);

Subscriber-side:
Create Subscription sub1 Connection 'dbname=postgres' Publication pub1, pub2;

We want to prohibit such cases. So, it would be better to check at the
time of 'Create Subscription' to validate such combinations and
prohibit them. To achieve that we extended the existing function
pg_get_publication_tables() and view pg_publication_tables to expose
the column list and verify such a combination. We primarily need
column list information for this prohibition but it appeared natural
to expose the row filter.

As mentioned in my previous email, we can fetch the required
information directly from system table pg_publication_rel and extend
the query in fetch_table_list to achieve the desired purpose but
extending the existing function/view for this appears to be a simpler
way.

>  Even more to the point, how can we
> have a subscriber do that by relying on view columns that don't exist in
> older versions?
>

We need a version check like (if
(walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000)) for that.

>  I'm also quite concerned about anything that involves
> subscribers examining row filter conditions; that sounds like a pretty
> direct route to bugs involving unsolvability and the halting problem.
>

We examine only the column list for the purpose of this prohibition.

-- 
With Regards,
Amit Kapila.