InvokeObjectPostAlterHook() vs. CommandCounterIncrement()

objectaccess.h has this to say:

* 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 SnapshotNow and SnapshotSelf to get the old and new
* versions of the tuple.

That's a clever design, but it precludes CommandCounterIncrement() appearing
between an applicable catalog change and the InvokeObjectPostAlterHook() call.
Otherwise, both SnapshotNow and SnapshotSelf see the new version. I find at
least one violation of that requirement. AlterTSConfiguration() calls
makeConfigurationDependencies(), which typically issues a CCI, before
InvokeObjectPostAlterHook(). A second example, arguably harmless, is
AlterEnum(). It can get a CCI through RenumberEnumType(), so SnapshotNow sees
the post-renumbered entries while SnapshotSelf sees those plus the added one.
sepgsql does not currently intercede for text search configurations or enums,
so only a third-party object_access_hook consumer could encounter the problem.

This is a dangerously-easy problem to introduce; unrelated development could
add a CCI to some intermediate code in a DDL implementation and quietly break
object_access_hook consumers. Can we detect that mistake? One possibility is
to call a macro, say StartObjectAlter(), before the first catalog modification
associated with a particular hook invocation. This macro would stash the
current CID. Then, RunObjectPostAlterHook() would elog(ERROR) if the CID had
changed since that moment. But how would one actually fix the code after such
a check fires? It's rarely easy to avoid adding a CCI.

I'm thinking we would be better off saying the firing point needs to appear
right after the heap_{insert,update,delete} + CatalogUpdateIndexes(). The
hooks already have the form of applying to a single catalog row rather than a
complete DDL operation, and many of the firing sites are close to that
placement already. If these hooks will need to apply to a larger operation, I
think that mandates a different means to reliably expose the before/after
object states.

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com