Re: Bugs in pgoutput.c

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Thu, Jan 6, 2022 at 3:42 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Also ... maybe I'm not looking in the right place, but I do not
>> see anything anywhere in logical decoding that is taking any lock
>> on the relation being processed. How can that be safe?

> We don't need to acquire a lock on relation while decoding changes
> from WAL because it uses a historic snapshot to build a relcache entry
> and all the later changes to the rel are absorbed while decoding WAL.

That might be okay for the system catalog entries, but I don't see
how it prevents some other session from dropping the table entirely,
thereby causing the on-disk storage to go away. Is it guaranteed
that logical decoding will never try to fetch any on-disk data?
(I can sort of believe that that might be true, but there are scary
corner cases for toasted data, such as an UPDATE that carries forward
a pre-existing toast datum.)

> * Would it be better if we move all the initialization done by patch
> in get_rel_sync_entry() to a separate function as I expect future
> patches might need to reset more things?

Don't see that it helps particularly.

> + * *during* a callback if we do any syscache or table access in the
> + * callback.

> As we don't take locks on tables, can invalidation events be accepted
> during table access? I could be missing something but I see relation.c
> accepts invalidation messages only when lock mode is not 'NoLock'.

The core point here is that you're assuming that NO code path taken
during logical decoding would try to take a lock. I don't believe it,
at least not unless you can point me to some debugging cross-check that
guarantees it.

Given that we're interested in historic not current snapshots, I can
buy that it might be workable to manage syscache invalidations totally
differently than the way it's done in normal processing, in which case
(*if* it's done like that) maybe no invals would need to be recognized
while an output plugin is executing. But (a) the comment here is
entirely wrong if that's so, and (b) I don't see anything in inval.c
that makes it work differently.

regards, tom lane