Andres Freund <andres@anarazel.de> writes:
> On 2019-08-17 01:43:45 -0400, Tom Lane wrote:
>> Yeah ... see the discussion leading up to 9c703c169,
>> https://www.postgresql.org/message-id/flat/19465.1541636036%40sss.pgh.pa.us
>> We didn't pull the trigger on removing the CMD_DELETE exception here,
>> but I think the handwriting has been on the wall for some time.
> ISTM there's a few different options here:
> 1a) We build all index infos, unconditionally. As argued in the thread
> you reference, future tableams may eventually require that anyway,
> by doing more proactive index maintenance somehow. Currently there's
> however no support for such AMs via tableam (mostly because I wasn't
> sure how exactly that'd look, and none of the already in-development
> AMs needed it).
> 2a) We separate acquisition of index locks from ExecOpenIndices(), and
> acquire index locks even for CMD_DELETE. Do so either during
> executor startup, or as part of AcquireExecutorLocks() (the latter
> on the basis that parsing/planning would have required the locks
> already).
> There's also corresponding *b) options, where we only do additional work
> for CMD_DELETE if wal_level = logical, and the table has a replica
> identity requiring use of the index during deleteions. But I think
> that's clearly enough a bad idea that we can just dismiss it out of
> hand.
> 3) Remove the CheckRelationLockedByMe() assert from
> ExtractReplicaIdentity(), at least for 12. I don't think this is an
> all that convicing option, but it'd reduce churn relatively late in
> beta.
> 4) Add a index_open(RowExclusiveLock) to ExtractReplicaIdentity(). That
> seems very unconvincing to me, because we'd do so for every row.
As far as 4) goes, I think the code in ExtractReplicaIdentity is pretty
duff anyway, because it doesn't bother to check for the defined failure
return for RelationIdGetRelation. But if we're concerned about the
cost of recalculating this stuff per-row, couldn't we cache it a little
better? It should be safe to assume the set of index columns isn't
changing intra-query.
... in fact, isn't all the infrastructure for that present already?
Why is this code looking directly at the index at all, rather than
using the relcache's rd_idattr bitmap?
I suspect we'll have to do 1a) eventually anyway, but this particular
problem seems like it has a better solution. Will try to produce a
patch in a bit.
regards, tom lane