Re: BUG #14785: Logical replication does not work after adding a column. Bug?

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> On 9/25/17 15:16, Andres Freund wrote:
>> This'll accept tablenames like pg_temp_1foo, right? Might be worth
>> being a bit narrower in the test.

> Committed with that change. Thanks.

This patch is using the wrong approach entirely. Every other place in
the backend that is trying to exclude temp relations uses a test on the
containing namespace, not the relname. You should be using
RELATION_IS_OTHER_TEMP(), or if there's good reason to decide this
without opening the relation, maybe you could get away with
isOtherTempNamespace(get_rel_namespace(relid)).

I also note that as committed, the patch will dump core on a
concurrently-dropped relation, because get_rel_name returns NULL
under such circumstances.

BTW, get_rel_sync_entry has some other serious problems: it is not being
at all careful about whether persistent data structures are left in sane
states if it gets an error partway through. In particular it'll leave
behind a new hash entry in entirely-unknown state, and if LoadPublications
gets an error, it will also leave a time bomb behind in the form of
not nil, but already-list-freed, data->publications. And I sure do not
understand why a single static variable publications_valid is being used
to remember validity of data->publications ... couldn't there be more
than one of those?

regards, tom lane