Hi,
On 12/13/22 2:50 PM, Robert Haas wrote:
On Tue, Dec 13, 2022 at 5:49 AM Drouvot, Bertrand
<bertranddrouvot...@gmail.com> wrote:
I think the real problem here is that
RelationIsAccessibleInLogicalDecoding is returning *the wrong answer*
when the relation is a user-catalog table. It does so because it
relies on RelationIsUsedAsCatalogTable, and that macro relies on
checking whether the reloptions include user_catalog_table.
[ confusion ]
Sorry, I meant: RelationIsAccessibleInLogicalDecoding is returning
*the wrong answer* when the relation is an *INDEX*.
Yeah, agree. Will fix it in the next patch proposal (adding the index case in
it as you proposed up-thread).
It would be very helpful if there were some place to refer to that
explained the design decisions here, like why the feature we're trying
to get requires this infrastructure around indexes to be added. It
could be in the commit messages, an email message, a README, or
whatever, but right now I don't see it anywhere in here.
Like adding something around those lines in the commit message?
"
On a primary database, any catalog rows that may be needed by a logical
decoding replication slot are not removed.
This is done thanks to the catalog_xmin associated with the logical replication
slot.
With logical decoding on standby, in the following cases:
- hot_standby_feedback is off
- hot_standby_feedback is on but there is no a physical slot between the
primary and the standby. Then, hot_standby_feedback will work, but only while
the connection is alive (for example a node restart would break it)
Then the primary may delete system catalog rows that could be needed by the
logical decoding on the standby. Then, it’s mandatory to identify those rows
and invalidate the slots that may need them if any.
"
This is very helpful, yes. I think perhaps we need to work some of
this into the code comments someplace, but getting it into the commit
message would be a good first step.
Thanks, will do.
What I infer from the above is that the overall design looks like this:
- We want to enable logical decoding on standbys, but replay of WAL
from the primary might remove data that is needed by logical decoding,
causing replication conflicts much as hot standby does.
- Our chosen strategy for dealing with this type of replication slot
is to invalidate logical slots for which needed data has been removed.
- To do this we need the latestRemovedXid for each change, just as we
do for physical replication conflicts, but we also need to know
whether any particular change was to data that logical replication
might access.
- We can't rely on the standby's relcache entries for this purpose in
any way, because the WAL record that causes the problem might be
replayed before the standby even reaches consistency. (Is this true? I
think so.)
- Therefore every WAL record that potentially removes data from the
index or heap must carry a flag indicating whether or not it is one
that might be accessed during logical decoding.
Does that sound right?
Yeah, that sounds all right to me.
One option could be to add my proposed wording in the commit message and put
your wording above in a README.
It seems kind of unfortunate to have to add payload to a whole bevy of
record types for this feature. I think it's worth it, both because the
feature is extremely important,
Agree and I don't think that there is other option than adding some payload in
some WAL records (at the very beginning the proposal was to periodically log a
new record
that announces the current catalog xmin horizon).
and also because there aren't any
record types that fall into this category that are going to be emitted
so frequently as to make it a performance problem.
+1
If no objections from your side, I'll submit a patch proposal by tomorrow,
which:
- get rid of IndexIsAccessibleInLogicalDecoding
- let RelationIsAccessibleInLogicalDecoding deals with the index case
- takes care of the padding where the new bool is added
- convert this new bool to a flag for the xl_heap_visible case (adding a new
bit to the already existing flag)
- Add my proposed wording above to the commit message
- Add your proposed wording above in a README
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
