From: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Pg Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: bogus: logical replication rows/cols combinations |
Date: | 2022-05-16 13:20:33 |
Message-ID: | 202205161320.butjtubabvgj@alvherre.pgsql |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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/
From | Date | Subject | |
---|---|---|---|
Next Message | Amul Sul | 2022-05-16 13:23:04 | Re: Convert macros to static inline functions |
Previous Message | houzj.fnst@fujitsu.com | 2022-05-16 12:34:25 | RE: bogus: logical replication rows/cols combinations |