public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alvaro Herrera <[email protected]>
To: Amit Kapila <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Pg Hackers <[email protected]>
Subject: Re: bogus: logical replication rows/cols combinations
Date: Mon, 16 May 2022 15:20:33 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAA4eK1+UCfKUaf41H6zHS2qR4CZoVtZe64yx_T-JVPSb1pdw_w@mail.gmail.com>
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/
view thread (63+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: bogus: logical replication rows/cols combinations
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox