Re: oat_post_create expected behavior

On Mon, Jun 6, 2022 at 1:35 PM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> Out of curiosity, why not? The proposed patch only runs it if the
> object access hook is set. Do you see a situation where it would be
> confusing that an earlier DDL change is visible? And if so, would it
> make more sense to call CCI unconditionally?

Well, I think that a fair amount of work has been done previously to
cut down on unnecessary CCIs. I suspect Tom in particular is likely to
object to adding a whole bunch more of them, and I think that
objection would have some merit.

I definitely think if we were going to do it, it would need to be
unconditional. Otherwise I think we'll end up with bugs, because
nobody's going to go test all of that code with and without an object
access hook installed every time they tweak some DDL-related code.

> Also, would it ever be reasonable for such a hook to call CCI itself?
> As you say, it could use SnapshotSelf, but sometimes you might want to
> call routines that assume they can use an MVCC snapshot. This question
> applies to the OAT_POST_ALTER hook as well as OAT_POST_CREATE.

I definitely wouldn't want to warrant that it doesn't break anything.
I think the extension can do it at its own risk, but I wouldn't
recommend it.

OAT_POST_ALTER is unlike OAT_POST_CREATE in that OAT_POST_ALTER
documents that it should be called after a CCI, whereas
OAT_POST_CREATE does not make a representation either way.

> > Possibly there is some work that could be done to ensure
> > consistent placement of the calls to post-create hooks so that either
> > all of them happen before, or all of them happen after, a CCI has
> > occurred, but I'm not sure whether or not that is feasible.
>
> I like the idea of having a test in place so that we at least know when
> something changes. Otherwise it would be pretty easy to break an
> extension by adjusting some code.

Sure. I find writing meaningful tests for this kind of stuff hard, but
there are plenty of people around here who are better at figuring out
how to test obscure scenarios than I.

> > Currently,
> > I don't think we promise anything about whether a post-create hook
> > call will occur before or after a CCI, which is why
> > sepgsql_schema_post_create(), sepgsql_schema_post_create(), and
> > sepgsql_attribute_post_create() perform a catalog scan using
> > SnapshotSelf, while sepgsql_database_post_create() uses
> > get_database_oid(). You might want to adopt a similar technique.
>
> It would be good to document this a little better though:
>
> * OAT_POST_CREATE should be invoked just after the object is created.
> * Typically, this is done after inserting the primary catalog records
> and
> * associated dependencies.
>
> doesn't really give any guidance, while the comment for alter does:
>
> * OAT_POST_ALTER should be invoked just after the object is altered,
> * but before the command counter is incremented. An extension using
> the
> * hook can use a current MVCC snapshot to get the old version of the
> tuple,
> * and can use SnapshotSelf to get the new version of the tuple.

Yeah, that comment could be made more clear.

--
Robert Haas
EDB: http://www.enterprisedb.com