Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions?

Hi,

On 2019-11-13 15:58:28 +0900, Michael Paquier wrote:
> On Tue, Nov 12, 2019 at 10:33:16AM -0800, Andres Freund wrote:
> > On 2019-11-12 16:25:06 +0100, Daniel Gustafsson wrote:
> >> On 11 Nov 2019, at 09:32, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >>
> >>> This part has resulted in 75c1921, and we could just change
> >>> DecodeMultiInsert() so as if there is no tuple data then we'd just
> >>> leave. However, I don't feel completely comfortable with that either
> >>> as it would be nice to still check that for normal relations we
> >>> *always* have a FPW available.
> >>
> >> XLH_INSERT_CONTAINS_NEW_TUPLE will only be set in case of catalog relations
> >> IIUC (that is, non logically decoded relations), so it seems to me that we can
> >> have a fastpath out of DecodeMultiInsert() which inspects that flag without
> >> problems. Is this proposal along the lines of what you were thinking?
> >
> > Maybe I'm missing something, but it's the opposite, no?
>
> > bool need_tuple_data = RelationIsLogicallyLogged(relation);
>
> Yep. This checks after IsCatalogRelation().
>
> > ...
> > if (need_tuple_data)
> > xlrec->flags |= XLH_INSERT_CONTAINS_NEW_TUPLE;
> >
> > or am I misunderstanding what you mean?
>
> Not sure that I can think about a good way to properly track if the
> new tuple data is associated to a catalog or not, aka if the data is
> expected all the time or not to get a good sanity check when doing the
> multi-insert decoding. We could extend xl_multi_insert_tuple with a
> flag to track that, but that seems like an overkill for a simple
> sanity check..

My point was that I think there must be negation missing in Daniel's
statement - XLH_INSERT_CONTAINS_NEW_TUPLE will only be set if *not* a
catalog relation. But Daniel's statement says exactly the opposite, at
least by my read.

I can't follow what you're trying to get at in this sub discussion - why
would we want to sanity check something about catalog tables here? Given
that XLH_INSERT_CONTAINS_NEW_TUPLE is set exactly when the table is not
a catalog table, that seems entirely superfluous?

Greetings,

Andres Freund