Alvaro Herrera wrote:
> Correct. The only difficulty here is that we would need to pass down
> the fact that we know for certain that this is only a locking Multixact.
> There are some callers that go to it indirectly via MultiXactIdWait or
> MultiXactIdExpand, but now that I look I think it's fine for those to
> pass false (i.e. assume there might be an update and disable the
> optimization), since those aren't hot compared to the other cases.
>
> This patch implements this idea, but I haven't tested it much beyond
> compiling and ensuring it passes the existing tests.
.. and it turns out it doesn't work. To be really effective, we need
MultiXactIdIsRunning to be passed down the flag too, so it can pass it
to GetMultiXactIdMembers.
One other thought is that MultiXactIdIsRunning and GetMultiXactIdMembers
are public functions, so this patch would represent an API change in
9.3. I doubt any external modules would be relying on these functions,
but there's so many care and thought put into avoiding API changes on
released versions that I'm nervous about doing it here. So I think we'd
need to provide a compatibility shim to avoid that.
(I generally dislike to keep compatibility stuff forever, so I would
provide this backward-compatible functions in 9.3 only. Anyone using it
would have to fix the code and recompile for 9.4+. This means a #ifdef
in code meant to work on top of both 9.3 and 9.4. Anyone opines
otherwise?)
The other idea is to just not backpatch this.
Other than that, this patch implements the optimization suggested here.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
*** a/contrib/pgrowlocks/pgrowlocks.c
--- b/contrib/pgrowlocks/pgrowlocks.c
***************
*** 168,174 **** pgrowlocks(PG_FUNCTION_ARGS)
allow_old = !(infomask & HEAP_LOCK_MASK) &&
(infomask & HEAP_XMAX_LOCK_ONLY);
! nmembers = GetMultiXactIdMembers(xmax, &members, allow_old);
if (nmembers == -1)
{
values[Atnum_xids] = "{0}";
--- 168,175 ----
allow_old = !(infomask & HEAP_LOCK_MASK) &&
(infomask & HEAP_XMAX_LOCK_ONLY);
! nmembers = GetMultiXactIdMembers(xmax, &members, allow_old,
! false);
if (nmembers == -1)
{
values[Atnum_xids] = "{0}";
*** a/src/backend/access/heap/heapam.c
--- b/src/backend/access/heap/heapam.c
***************
*** 3987,3993 **** l3:
* the case, HeapTupleSatisfiesUpdate would have returned
* MayBeUpdated and we wouldn't be here.
*/
! nmembers = GetMultiXactIdMembers(xwait, &members, false);
for (i = 0; i < nmembers; i++)
{
--- 3987,3995 ----
* the case, HeapTupleSatisfiesUpdate would have returned
* MayBeUpdated and we wouldn't be here.
*/
! nmembers =
! GetMultiXactIdMembers(xwait, &members, false,
! HEAP_XMAX_IS_LOCKED_ONLY(infomask));
for (i = 0; i < nmembers; i++)
{
***************
*** 4008,4014 **** l3:
}
}
! pfree(members);
}
/*
--- 4010,4017 ----
}
}
! if (members)
! pfree(members);
}
/*
***************
*** 4157,4163 **** l3:
* been the case, HeapTupleSatisfiesUpdate would have returned
* MayBeUpdated and we wouldn't be here.
*/
! nmembers = GetMultiXactIdMembers(xwait, &members, false);
if (nmembers <= 0)
{
--- 4160,4168 ----
* been the case, HeapTupleSatisfiesUpdate would have returned
* MayBeUpdated and we wouldn't be here.
*/
! nmembers =
! GetMultiXactIdMembers(xwait, &members, false,
! HEAP_XMAX_IS_LOCKED_ONLY(infomask));
if (nmembers <= 0)
{
***************
*** 4626,4632 **** l5:
* MultiXactIdExpand if we weren't to do this, so this check is not
* incurring extra work anyhow.
*/
! if (!MultiXactIdIsRunning(xmax))
{
if (HEAP_XMAX_IS_LOCKED_ONLY(old_infomask) ||
TransactionIdDidAbort(MultiXactIdGetUpdateXid(xmax,
--- 4631,4637 ----
* MultiXactIdExpand if we weren't to do this, so this check is not
* incurring extra work anyhow.
*/
! if (!MultiXactIdIsRunning(xmax, HEAP_XMAX_IS_LOCKED_ONLY(old_infomask)))
{
if (HEAP_XMAX_IS_LOCKED_ONLY(old_infomask) ||
TransactionIdDidAbort(MultiXactIdGetUpdateXid(xmax,
***************
*** 5217,5223 **** GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask,
* We only use this in multis we just created, so they cannot be values
* pre-pg_upgrade.
*/
! nmembers = GetMultiXactIdMembers(multi, &members, false);
for (i = 0; i < nmembers; i++)
{
--- 5222,5228 ----
* We only use this in multis we just created, so they cannot be values
* pre-pg_upgrade.
*/
! nmembers = GetMultiXactIdMembers(multi, &members, false, false);
for (i = 0; i < nmembers; i++)
{
***************
*** 5293,5299 **** MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask)
* Since we know the LOCK_ONLY bit is not set, this cannot be a multi from
* pre-pg_upgrade.
*/
! nmembers = GetMultiXactIdMembers(xmax, &members, false);
if (nmembers > 0)
{
--- 5298,5304 ----
* Since we know the LOCK_ONLY bit is not set, this cannot be a multi from
* pre-pg_upgrade.
*/
! nmembers = GetMultiXactIdMembers(xmax, &members, false, false);
if (nmembers > 0)
{
***************
*** 5376,5382 **** Do_MultiXactIdWait(MultiXactId multi, MultiXactStatus status,
int remain = 0;
allow_old = !(infomask & HEAP_LOCK_MASK) && HEAP_XMAX_IS_LOCKED_ONLY(infomask);
! nmembers = GetMultiXactIdMembers(multi, &members, allow_old);
if (nmembers >= 0)
{
--- 5381,5388 ----
int remain = 0;
allow_old = !(infomask & HEAP_LOCK_MASK) && HEAP_XMAX_IS_LOCKED_ONLY(infomask);
! nmembers = GetMultiXactIdMembers(multi, &members, allow_old,
! HEAP_XMAX_IS_LOCKED_ONLY(infomask));
if (nmembers >= 0)
{
*** a/src/backend/access/transam/multixact.c
--- b/src/backend/access/transam/multixact.c
***************
*** 391,397 **** MultiXactIdExpand(MultiXactId multi, TransactionId xid, MultiXactStatus status)
* caller of this function does a check that the multixact is no longer
* running.
*/
! nmembers = GetMultiXactIdMembers(multi, &members, false);
if (nmembers < 0)
{
--- 391,397 ----
* caller of this function does a check that the multixact is no longer
* running.
*/
! nmembers = GetMultiXactIdMembers(multi, &members, false, false);
if (nmembers < 0)
{
***************
*** 477,483 **** MultiXactIdExpand(MultiXactId multi, TransactionId xid, MultiXactStatus status)
* a pg_upgraded share-locked tuple.
*/
bool
! MultiXactIdIsRunning(MultiXactId multi)
{
MultiXactMember *members;
int nmembers;
--- 477,483 ----
* a pg_upgraded share-locked tuple.
*/
bool
! MultiXactIdIsRunning(MultiXactId multi, bool isLockOnly)
{
MultiXactMember *members;
int nmembers;
***************
*** 489,495 **** MultiXactIdIsRunning(MultiXactId multi)
* "false" here means we assume our callers have checked that the given
* multi cannot possibly come from a pg_upgraded database.
*/
! nmembers = GetMultiXactIdMembers(multi, &members, false);
if (nmembers < 0)
{
--- 489,495 ----
* "false" here means we assume our callers have checked that the given
* multi cannot possibly come from a pg_upgraded database.
*/
! nmembers = GetMultiXactIdMembers(multi, &members, false, isLockOnly);
if (nmembers < 0)
{
***************
*** 1029,1038 **** GetNewMultiXactId(int nmembers, MultiXactOffset *offset)
* the value currently known as the next to assign, raise an error. Previously
* these also returned -1, but since this can lead to the wrong visibility
* results, it is dangerous to do that.
*/
int
GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
! bool allow_old)
{
int pageno;
int prev_pageno;
--- 1029,1043 ----
* the value currently known as the next to assign, raise an error. Previously
* these also returned -1, but since this can lead to the wrong visibility
* results, it is dangerous to do that.
+ *
+ * onlyLock must be set to true if caller is certain that the given multi
+ * is used only to lock tuples; can be false without loss of correctness,
+ * but passing a true means we can return quickly without checking for
+ * old updates.
*/
int
GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
! bool allow_old, bool onlyLock)
{
int pageno;
int prev_pageno;
***************
*** 1066,1071 **** GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
--- 1071,1089 ----
MultiXactIdSetOldestVisible();
/*
+ * If we know the multi is used only for locking and not for updates,
+ * then we can skip checking if the value is older than our oldest
+ * visible multi. It cannot possibly still be running.
+ */
+ if (onlyLock &&
+ MultiXactIdPrecedes(multi, OldestVisibleMXactId[MyBackendId]))
+ {
+ debug_elog2(DEBUG2, "GetMembers: a locker-only multi is too old");
+ *members = NULL;
+ return -1;
+ }
+
+ /*
* We check known limits on MultiXact before resorting to the SLRU area.
*
* An ID older than MultiXactState->oldestMultiXactId cannot possibly be
***************
*** 2507,2513 **** pg_get_multixact_members(PG_FUNCTION_ARGS)
multi = palloc(sizeof(mxact));
/* no need to allow for old values here */
! multi->nmembers = GetMultiXactIdMembers(mxid, &multi->members, false);
multi->iter = 0;
tupdesc = CreateTemplateTupleDesc(2, false);
--- 2525,2532 ----
multi = palloc(sizeof(mxact));
/* no need to allow for old values here */
! multi->nmembers = GetMultiXactIdMembers(mxid, &multi->members, false,
! false);
multi->iter = 0;
tupdesc = CreateTemplateTupleDesc(2, false);
*** a/src/backend/utils/time/tqual.c
--- b/src/backend/utils/time/tqual.c
***************
*** 565,571 **** HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
*/
if ((tuple->t_infomask & (HEAP_XMAX_EXCL_LOCK |
HEAP_XMAX_KEYSHR_LOCK)) &&
! MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
return HeapTupleBeingUpdated;
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
--- 565,571 ----
*/
if ((tuple->t_infomask & (HEAP_XMAX_EXCL_LOCK |
HEAP_XMAX_KEYSHR_LOCK)) &&
! MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), true))
return HeapTupleBeingUpdated;
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
***************
*** 575,581 **** HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
{
! if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
return HeapTupleBeingUpdated;
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
--- 575,581 ----
xmax = HeapTupleGetUpdateXid(tuple);
if (!TransactionIdIsValid(xmax))
{
! if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), false))
return HeapTupleBeingUpdated;
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
***************
*** 590,596 **** HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
return HeapTupleInvisible; /* updated before scan started */
}
! if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
return HeapTupleBeingUpdated;
if (TransactionIdDidCommit(xmax))
--- 590,596 ----
return HeapTupleInvisible; /* updated before scan started */
}
! if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), false))
return HeapTupleBeingUpdated;
if (TransactionIdDidCommit(xmax))
***************
*** 1158,1164 **** HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
*/
if ((tuple->t_infomask & (HEAP_XMAX_EXCL_LOCK |
HEAP_XMAX_KEYSHR_LOCK)) &&
! MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
return HEAPTUPLE_LIVE;
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
--- 1158,1165 ----
*/
if ((tuple->t_infomask & (HEAP_XMAX_EXCL_LOCK |
HEAP_XMAX_KEYSHR_LOCK)) &&
! MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
! true))
return HEAPTUPLE_LIVE;
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
***************
*** 1185,1191 **** HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
{
TransactionId xmax;
! if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
{
/* already checked above */
Assert(!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask));
--- 1186,1192 ----
{
TransactionId xmax;
! if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), false))
{
/* already checked above */
Assert(!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask));
*** a/src/include/access/multixact.h
--- b/src/include/access/multixact.h
***************
*** 82,91 **** extern MultiXactId MultiXactIdCreate(TransactionId xid1,
extern MultiXactId MultiXactIdExpand(MultiXactId multi, TransactionId xid,
MultiXactStatus status);
extern MultiXactId ReadNextMultiXactId(void);
! extern bool MultiXactIdIsRunning(MultiXactId multi);
extern void MultiXactIdSetOldestMember(void);
extern int GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **xids,
! bool allow_old);
extern bool MultiXactIdPrecedes(MultiXactId multi1, MultiXactId multi2);
extern void AtEOXact_MultiXact(void);
--- 82,91 ----
extern MultiXactId MultiXactIdExpand(MultiXactId multi, TransactionId xid,
MultiXactStatus status);
extern MultiXactId ReadNextMultiXactId(void);
! extern bool MultiXactIdIsRunning(MultiXactId multi, bool isLockOnly);
extern void MultiXactIdSetOldestMember(void);
extern int GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **xids,
! bool allow_old, bool isLockOnly);
extern bool MultiXactIdPrecedes(MultiXactId multi1, MultiXactId multi2);
extern void AtEOXact_MultiXact(void);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
