Hi,
On 1/6/23 4:40 AM, Andres Freund wrote:
Hi,
Thomas, there's one point at the bottom wrt ConditionVariables that'd be
interesting for you to comment on.
On 2023-01-05 16:15:39 -0500, Robert Haas wrote:
On Tue, Jan 3, 2023 at 2:42 AM Drouvot, Bertrand
<bertranddrouvot...@gmail.com> wrote:
Please find attached v36, tiny rebase due to 1de58df4fe.
0001 looks committable to me now, though we probably shouldn't do that
unless we're pretty confident about shipping enough of the rest of
this to accomplish something useful.
Thanks for your precious help reaching this state!
Cool!
ISTM that the ordering of patches isn't quite right later on. ISTM that it
doesn't make sense to introduce working logic decoding without first fixing
WalSndWaitForWal() (i.e. patch 0006). What made you order the patches that
way?
Idea was to ease the review: 0001 to 0005 to introduce the feature and 0006 to
deal
with this race condition.
I thought it would be easier to review that way (given the complexity of "just"
adding the
feature itself).
0001:
4. 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.
The startup process can't access catalog contents in the first place, so the
consistency issue is secondary.
Thanks for pointing out, I'll update the commit message.
ISTM that the commit message omits a fairly significant portion of the change:
The introduction of indisusercatalog / the reason for its introduction.
Agree, will do (or create a dedicated path as you are suggesting below).
Why is indisusercatalog stored as "full" column, whereas we store the fact of
table being used as a catalog table in a reloption? I'm not adverse to moving
to a full column, but then I think we should do the same for tables.
Earlier version of the patches IIRC sourced the "catalogness" from the
relation. What lead you to changing that? I'm not saying it's wrong, just not
sure it's right either.
That's right it's started retrieving this information from the relation.
Then, Robert made a comment in [1] saying it's not safe to call
table_open() while holding a buffer lock.
Then, I worked on other options and submitted the current one.
While reviewing 0001, Robert's also thought of it (see [2])) and finished with:
"
So while I do not really like the approach of storing the same
property in different ways for tables and for indexes, it's also not
really obvious to me how to do better.
"
That's also my thought.
It'd be good to introduce cross-checks that indisusercatalog is set
correctly. RelationGetIndexList() seems like a good candidate.
Good point, will look at it.
I'd probably split the introduction of indisusercatalog into a separate patch.
You mean, completely outside of this patch series or a sub-patch in this series?
If the former, I'm not sure it would make sense outside of the current context.
Why was HEAP_DEFAULT_USER_CATALOG_TABLE introduced in this patch?
To help in case of reset on the table (ensure the default gets also propagated
to the indexes).
I wonder if we instead should compute a relation's "catalogness" in the
relcache. That'd would have the advantage of making
RelationIsUsedAsCatalogTable() cheaper and working for all kinds of
relations.
Any idea on where and how you'd do that? (that's one option I explored in vain
before
submitting the current proposal).
It does look like that's also an option explored by Robert in [2]:
"
Yet a third way is to have the index fetch the flag from
the associated table, perhaps when the relcache entry is built. But I
see no existing precedent for that in RelationInitIndexAccessInfo,
which I think is where it would be if we had it -- and that makes me
suspect that there might be good reasons why this isn't actually safe.
"
VISIBILITYMAP_ON_CATALOG_ACCESSIBLE_IN_LOGICAL_DECODING is a very long
identifier. Given that the field in the xlog records is just named
isCatalogRel, any reason to not just name it correspondingly?
Agree, VISIBILITYMAP_IS_CATALOG_REL maybe?
I'll look at the other comments too and work on/reply later on.
[1]:
https://www.postgresql.org/message-id/CA%2BTgmobgOLH-JpBoBSdu4i%2BsjRdgwmDEZGECkmowXqQgQL6WhQ%40mail.gmail.com
[2]:
https://www.postgresql.org/message-id/CA%2BTgmoY0df9X%2B5ENg8P0BGj0odhM45sdQ7kB4JMo4NKaoFy-Vg%40mail.gmail.com
Thanks for your help,
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
