Date: Mon, 16 May 2022 15:20:33 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
	Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: bogus: logical replication rows/cols combinations
Message-ID: <202205161320.butjtubabvgj@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAA4eK1+UCfKUaf41H6zHS2qR4CZoVtZe64yx_T-JVPSb1pdw_w@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202205161320.butjtubabvgj%40alvherre.pgsql>
Precedence: bulk

On 2022-May-16, Amit Kapila wrote:

> > Agreed. If we can get columnlist and rowfilter from pg_publication_tables, it
> > will be more convenient. And I think users that want to fetch the columnlist
> > and rowfilter of table can also benefit from it.
> 
> After the change for this, we will give an error on combining
> publications where one of the publications specifies all columns in
> the table and the other doesn't provide any columns. We should not
> give an error as both mean all columns.

But don't we need to behave the same way for both column lists and row
filters?  I understand that some cases with different row filters for
different publications have shown to have weird behavior, so I think
it'd make sense to restrict it in the same way.  That would allow us to
extend the behavior in a sensible way when we develop that, instead of
setting in stone now behavior that we regret later.

> Few  comments:
> =================
> 1.
> postgres=# select * from pg_publication_tables;
>  pubname | schemaname | tablename | columnlist | rowfilter
> ---------+------------+-----------+------------+-----------
>  pub1    | public     | t1        |            |
>  pub2    | public     | t1        | 1 2        | (c3 < 10)
> (2 rows)
> 
> I think it is better to display column names for columnlist in the
> exposed view similar to attnames in the pg_stats_ext view. I think
> that will make it easier for users to understand this information.

+1

> I think we should change the "descr" to something like: 'get
> information of tables in a publication'

+1

> 3.
> +
> + /*
> + * We only throw a warning here so that the subcription can still be
> + * created and let user aware that something is going to fail later and
> + * they can fix the publications afterwards.
> + */
> + if (list_member(tablelist, rv))
> + ereport(WARNING,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot use different column lists for table \"%s.%s\" in
> different publications",
> +    nspname, relname));
> 
> Can we extend this comment to explain the case where after Alter
> Publication, if the user dumps and restores back the subscription,
> there is a possibility that "CREATE SUBSCRIPTION" won't work if we
> give ERROR here instead of WARNING?

Yeah, and not only the comment — I think we need to have more in the
warning message itself.  How about:

ERROR:  cannot use different column lists for table "..." in different publications
DETAIL:  The subscription "..." cannot currently be used for replication.


I think this whole affair is a bit sad TBH and I'm sure it'll give us
some grief -- similar to replication slots becoming inactive and nobody
noticing.  A user changing a publication in a way that prevents some
replica from working and the warnings are hidden, they could have
trouble noticing that the replica is stuck.

But I have no better ideas.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/