Re: Minimal logical decoding on standbys

Hi,

On 1/11/23 9:27 PM, Andres Freund wrote:
> Hi,
>
> On 2023-01-06 10:52:06 +0100, Drouvot, Bertrand wrote:
>
> The problem I have with that is that I saw a lot of flakiness in the tests due
> to the race condition. So introducing them in that order just doesn't make a
> whole lot of sense to me.

You are right it does not make sense to introduce fixing the race condition after the TAP tests
and after introducing the decoding logic. I'll reorder the sub-patches.

> It's also something that can be committed
> independently, I think.

Right but could this race condition occur outside of the context of this new feature?

>> 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.
>
> The suggested path in earlier versions to avoid doing so was to make sure that
> we pass down the Relation for the table into the necessary functions. Did you
> explore that any further?

So, for gistXLogPageReuse() and _bt_delitems_delete() this is "easy" to pass the Heap Relation.
This is what was done in earlier versions of this patch series.

But we would need to define a way to propagate the Heap Relation for those 2 functions:

_bt_log_reuse_page()
vacuumRedirectAndPlaceholder()

When I first looked at it and saw the number of places where _bt_getbuf() is called
then I preferred to have a look to the current proposal.

I will give it another look, also because I just realized that it could be beneficial
for vacuumRedirectAndPlaceholder() too, as per this comment:

"
/* XXX: providing heap relation would allow more pruning */
vistest = GlobalVisTestFor(NULL);
"

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com