Hot Standby b-tree delete records review

btree_redo:
> case XLOG_BTREE_DELETE:
>
> /*
> * Btree delete records can conflict with standby queries. You
> * might think that vacuum records would conflict as well, but
> * we've handled that already. XLOG_HEAP2_CLEANUP_INFO records
> * provide the highest xid cleaned by the vacuum of the heap
> * and so we can resolve any conflicts just once when that
> * arrives. After that any we know that no conflicts exist
> * from individual btree vacuum records on that index.
> */
> {
> TransactionId latestRemovedXid = btree_xlog_delete_get_latestRemovedXid(record);
> xl_btree_delete *xlrec = (xl_btree_delete *) XLogRecGetData(record);
>
> /*
> * XXX Currently we put everybody on death row, because
> * currently _bt_delitems() supplies InvalidTransactionId.
> * This can be fairly painful, so providing a better value
> * here is worth some thought and possibly some effort to
> * improve.
> */
> ResolveRecoveryConflictWithSnapshot(latestRemovedXid, xlrec->node);
> }
> break;

The XXX comment is out-of-date, the latestRemovedXid value is calculated
by btree_xlog_delete_get_latestRemovedXid() nowadays.

If we're re-replaying the WAL record, for example after restarting the
standby server, btree_xlog_delete_get_latestRemovedXid() won't find the
deleted records and will return InvalidTransactionId. That's OK, because
until we reach again the point in WAL where we were before the restart,
we don't accept read-only connections so there's no-one to kill anyway,
but you do get a useless "Invalid latestRemovedXid reported, using
latestCompletedXid instead" message in the log (that shouldn't be
capitalized, BTW).

It would be nice to check if there's any potentially conflicting
read-only queries before calling
btree_xlog_delete_get_latestRemovedXid(), which can be quite expensive.

If the "Invalid latestRemovedXid reported, using latestCompletedXid
instead" message is going to happen commonly, I think it should be
downgraded to DEBUG1. If it's an unexpected scenario, it should be
upgraded to WARNING.

In btree_xlog_delete_get_latestRemovedXid:
> Assert(num_unused == 0);

Can't that happen as well in a re-replay scenario, if a heap item was
vacuumed away later on?

> /*
> * Note that if all heap tuples were LP_DEAD then we will be
> * returning InvalidTransactionId here. This seems very unlikely
> * in practice.
> */

If none of the removed heap tuples were present anymore, we currently
return InvalidTransactionId, which kills/waits out all read-only
queries. But if none of the tuples were present anymore, the read-only
queries wouldn't have seen them anyway, so ISTM that we should treat
InvalidTransactionId return value as "we don't need to kill anyone".

Why does btree_xlog_delete_get_latestRemovedXid() keep the
num_unused/num_dead/num_redirect counts, it doesn't actually do anything
with them.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com