Content-Type: multipart/mixed; boundary="------------yGm38uGpVMppwr9IvRHchYXP"
Message-ID: <5a0f78d1-0f56-439b-b4d3-2bae9d262c6c@oss.nttdata.com>
Date: Wed, 23 Oct 2024 01:19:37 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Refactor GetLockStatusData() by skipping unused backends and
 groups
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: pgsql-hackers@postgresql.org
References: <a0a00c44-31e9-4c67-9846-fb9636213ac9@oss.nttdata.com>
 <ZxYDgginZZGFPxqp@ip-10-97-1-34.eu-west-3.compute.internal>
Content-Language: en-US
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <ZxYDgginZZGFPxqp@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/5a0f78d1-0f56-439b-b4d3-2bae9d262c6c%40oss.nttdata.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------yGm38uGpVMppwr9IvRHchYXP
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 2024/10/21 16:32, Bertrand Drouvot wrote:
> A few random comments on it:

Thanks for the review!


> 1 ===
> 
> +               /* Skip backends with pid=0, as they don't hold fast-path locks */
> +               if (proc->pid == 0)
> +                       continue;
> 
> What about adding a few words in the comment that it represents prepared
> transactions? Or maybe add a new macro (say IS_PREPARED_TRANSACTION(proc)) and
> use it in the few places where we check for "PGPROC"->pid == 0 or "PGPROC"->pid != 0?

I understand that PGPROC entries with pid=0 are typically those not yet allocated to
any backends. Yes, as you mentioned, prepared transactions also have pid=0. However,
GetLockStatusData() loops up to ProcGlobal->allProcCount, which is MaxBackends plus
NUM_AUXILIARY_PROCS, excluding prepared transactions. Therefore, GetLockStatusData()
doesn't seem to check PGPROC entries for prepared transactions at all.

In proc.c
--------------
/* XXX allProcCount isn't really all of them; it excludes prepared xacts */
ProcGlobal->allProcCount = MaxBackends + NUM_AUXILIARY_PROCS;
--------------


> One remark about the comment, what about?
> 
> s/Skip unallocated groups/Skip groups without registered fast-path locks./?

I've updated the source comment accordingly.


> or at least add a "." at the end to be consistent with:
> 
> "/* Skip unallocated slots. */"

I removed the period at the end to match the usual convention in the codebase
for single-line comment.

I've attached v2 patch.


> 3 ===
> 
> One thing that worry me a bit is that we "lost" the FP_LOCK_SLOTS_PER_BACKEND usage,
> so that if there is a change on it (for wathever reason) then we probably need to
> be careful that the change would be reflected here too.
> 
> So, what about to add an Assert to check that we overall iterated over FP_LOCK_SLOTS_PER_BACKEND?

You mean adding an assertion check to ensure that the slot ID calculated by
FAST_PATH_SLOT() is less than FP_LOCK_SLOTS_PER_BACKEND? But GetLockStatusData()
already calls FAST_PATH_GET_BITS() right after FAST_PATH_SLOT(),
and FAST_PATH_GET_BITS() has an assertion that validates this. So, probably
we can consider that this check is already in place. If it’s still worth adding,
perhaps placing it inside the FAST_PATH_SLOT() macro could be an option...
Or current assertion check is enough? Thought?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

--------------yGm38uGpVMppwr9IvRHchYXP
Content-Type: text/plain; charset=UTF-8;
 name="v2-0001-Refactor-GetLockStatusData-to-skip-backends-group.patch"
Content-Disposition: attachment;
 filename*0="v2-0001-Refactor-GetLockStatusData-to-skip-backends-group.pa";
 filename*1="tch"
Content-Transfer-Encoding: base64

RnJvbSBiNTU2MjBjZjczZDEyZWY5NWRjMjFmM2I0MmFmYWEyNjQ1MDA2MTJkIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBGdWppaSBNYXNhbyA8ZnVqaWlAcG9zdGdyZXNxbC5v
cmc+CkRhdGU6IFN1biwgMjAgT2N0IDIwMjQgMTQ6NTE6MDcgKzA5MDAKU3ViamVjdDogW1BB
VENIIHYyXSBSZWZhY3RvciBHZXRMb2NrU3RhdHVzRGF0YSgpIHRvIHNraXAgYmFja2VuZHMv
Z3JvdXBzCiB3aXRob3V0IGZhc3QtcGF0aCBsb2Nrcy4KClByZXZpb3VzbHksIEdldExvY2tT
dGF0dXNEYXRhKCkgY2hlY2tlZCBhbGwgc2xvdHMgZm9yIGV2ZXJ5IGJhY2tlbmQKdG8gZ2F0
aGVyIGZhc3QtcGF0aCBsb2NrIGRhdGEsIHdoaWNoIGNvdWxkIGJlIGluZWZmaWNpZW50LiBU
aGlzIGNvbW1pdApyZWZhY3RvcnMgaXQgYnkgc2tpcHBpbmcgYmFja2VuZHMgd2l0aCBQSUQ9
MCAoc2luY2UgdGhleSBkb24ndCBob2xkCmZhc3QtcGF0aCBsb2NrcykgYW5kIHNraXBwaW5n
IGdyb3VwcyB3aXRoIG5vIHJlZ2lzdGVyZWQgZmFzdC1wYXRoIGxvY2tzLAppbXByb3Zpbmcg
ZWZmaWNpZW5jeS4KClRoaXMgcmVmYWN0b3JpbmcgaXMgcGFydGljdWxhcmx5IGJlbmVmaWNp
YWwsIGZvciBleGFtcGxlIHdoZW4KbWF4X2Nvbm5lY3Rpb25zIGFuZCBtYXhfbG9ja3NfcGVy
X3RyYW5zYWN0aW9uIGFyZSBzZXQgaGlnaCwKYXMgaXQgcmVkdWNlcyB1bm5lY2Vzc2FyeSBj
aGVja3MgYWNyb3NzIG51bWVyb3VzIHNsb3RzLgotLS0KIHNyYy9iYWNrZW5kL3N0b3JhZ2Uv
bG1nci9sb2NrLmMgfCA2NyArKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLS0tLS0KIDEg
ZmlsZSBjaGFuZ2VkLCAzOSBpbnNlcnRpb25zKCspLCAyOCBkZWxldGlvbnMoLSkKCmRpZmYg
LS1naXQgYS9zcmMvYmFja2VuZC9zdG9yYWdlL2xtZ3IvbG9jay5jIGIvc3JjL2JhY2tlbmQv
c3RvcmFnZS9sbWdyL2xvY2suYwppbmRleCAwOWE4YWMxNTc4Li40ZmNjYjcyNzdlIDEwMDY0
NAotLS0gYS9zcmMvYmFja2VuZC9zdG9yYWdlL2xtZ3IvbG9jay5jCisrKyBiL3NyYy9iYWNr
ZW5kL3N0b3JhZ2UvbG1nci9sb2NrLmMKQEAgLTM3MzEsNDQgKzM3MzEsNTUgQEAgR2V0TG9j
a1N0YXR1c0RhdGEodm9pZCkKIAlmb3IgKGkgPSAwOyBpIDwgUHJvY0dsb2JhbC0+YWxsUHJv
Y0NvdW50OyArK2kpCiAJewogCQlQR1BST0MJICAgKnByb2MgPSAmUHJvY0dsb2JhbC0+YWxs
UHJvY3NbaV07Ci0JCXVpbnQzMgkJZjsKKworCQkvKiBTa2lwIGJhY2tlbmRzIHdpdGggcGlk
PTAsIGFzIHRoZXkgZG9uJ3QgaG9sZCBmYXN0LXBhdGggbG9ja3MgKi8KKwkJaWYgKHByb2Mt
PnBpZCA9PSAwKQorCQkJY29udGludWU7CiAKIAkJTFdMb2NrQWNxdWlyZSgmcHJvYy0+ZnBJ
bmZvTG9jaywgTFdfU0hBUkVEKTsKIAotCQlmb3IgKGYgPSAwOyBmIDwgRlBfTE9DS19TTE9U
U19QRVJfQkFDS0VORDsgKytmKQorCQlmb3IgKHVpbnQzMiBnID0gMDsgZyA8IEZhc3RQYXRo
TG9ja0dyb3Vwc1BlckJhY2tlbmQ7IGcrKykKIAkJewotCQkJTG9ja0luc3RhbmNlRGF0YSAq
aW5zdGFuY2U7Ci0JCQl1aW50MzIJCWxvY2tiaXRzID0gRkFTVF9QQVRIX0dFVF9CSVRTKHBy
b2MsIGYpOwotCi0JCQkvKiBTa2lwIHVuYWxsb2NhdGVkIHNsb3RzLiAqLwotCQkJaWYgKCFs
b2NrYml0cykKKwkJCS8qIFNraXAgZ3JvdXBzIHdpdGhvdXQgcmVnaXN0ZXJlZCBmYXN0LXBh
dGggbG9ja3MgKi8KKwkJCWlmIChwcm9jLT5mcExvY2tCaXRzW2ddID09IDApCiAJCQkJY29u
dGludWU7CiAKLQkJCWlmIChlbCA+PSBlbHMpCisJCQlmb3IgKGludCBqID0gMDsgaiA8IEZQ
X0xPQ0tfU0xPVFNfUEVSX0dST1VQOyBqKyspCiAJCQl7Ci0JCQkJZWxzICs9IE1heEJhY2tl
bmRzOwotCQkJCWRhdGEtPmxvY2tzID0gKExvY2tJbnN0YW5jZURhdGEgKikKLQkJCQkJcmVw
YWxsb2MoZGF0YS0+bG9ja3MsIHNpemVvZihMb2NrSW5zdGFuY2VEYXRhKSAqIGVscyk7Ci0J
CQl9CisJCQkJTG9ja0luc3RhbmNlRGF0YSAqaW5zdGFuY2U7CisJCQkJdWludDMyCQlmID0g
RkFTVF9QQVRIX1NMT1QoZywgaik7CisJCQkJdWludDMyCQlsb2NrYml0cyA9IEZBU1RfUEFU
SF9HRVRfQklUUyhwcm9jLCBmKTsKIAotCQkJaW5zdGFuY2UgPSAmZGF0YS0+bG9ja3NbZWxd
OwotCQkJU0VUX0xPQ0tUQUdfUkVMQVRJT04oaW5zdGFuY2UtPmxvY2t0YWcsIHByb2MtPmRh
dGFiYXNlSWQsCi0JCQkJCQkJCSBwcm9jLT5mcFJlbElkW2ZdKTsKLQkJCWluc3RhbmNlLT5o
b2xkTWFzayA9IGxvY2tiaXRzIDw8IEZBU1RfUEFUSF9MT0NLTlVNQkVSX09GRlNFVDsKLQkJ
CWluc3RhbmNlLT53YWl0TG9ja01vZGUgPSBOb0xvY2s7Ci0JCQlpbnN0YW5jZS0+dnhpZC5w
cm9jTnVtYmVyID0gcHJvYy0+dnhpZC5wcm9jTnVtYmVyOwotCQkJaW5zdGFuY2UtPnZ4aWQu
bG9jYWxUcmFuc2FjdGlvbklkID0gcHJvYy0+dnhpZC5seGlkOwotCQkJaW5zdGFuY2UtPnBp
ZCA9IHByb2MtPnBpZDsKLQkJCWluc3RhbmNlLT5sZWFkZXJQaWQgPSBwcm9jLT5waWQ7Ci0J
CQlpbnN0YW5jZS0+ZmFzdHBhdGggPSB0cnVlOworCQkJCS8qIFNraXAgdW5hbGxvY2F0ZWQg
c2xvdHMgKi8KKwkJCQlpZiAoIWxvY2tiaXRzKQorCQkJCQljb250aW51ZTsKIAotCQkJLyoK
LQkJCSAqIFN1Y2Nlc3NmdWxseSB0YWtpbmcgZmFzdCBwYXRoIGxvY2sgbWVhbnMgdGhlcmUg
d2VyZSBubwotCQkJICogY29uZmxpY3RpbmcgbG9ja3MuCi0JCQkgKi8KLQkJCWluc3RhbmNl
LT53YWl0U3RhcnQgPSAwOworCQkJCWlmIChlbCA+PSBlbHMpCisJCQkJeworCQkJCQllbHMg
Kz0gTWF4QmFja2VuZHM7CisJCQkJCWRhdGEtPmxvY2tzID0gKExvY2tJbnN0YW5jZURhdGEg
KikKKwkJCQkJCXJlcGFsbG9jKGRhdGEtPmxvY2tzLCBzaXplb2YoTG9ja0luc3RhbmNlRGF0
YSkgKiBlbHMpOworCQkJCX0KIAotCQkJZWwrKzsKKwkJCQlpbnN0YW5jZSA9ICZkYXRhLT5s
b2Nrc1tlbF07CisJCQkJU0VUX0xPQ0tUQUdfUkVMQVRJT04oaW5zdGFuY2UtPmxvY2t0YWcs
IHByb2MtPmRhdGFiYXNlSWQsCisJCQkJCQkJCQkgcHJvYy0+ZnBSZWxJZFtmXSk7CisJCQkJ
aW5zdGFuY2UtPmhvbGRNYXNrID0gbG9ja2JpdHMgPDwgRkFTVF9QQVRIX0xPQ0tOVU1CRVJf
T0ZGU0VUOworCQkJCWluc3RhbmNlLT53YWl0TG9ja01vZGUgPSBOb0xvY2s7CisJCQkJaW5z
dGFuY2UtPnZ4aWQucHJvY051bWJlciA9IHByb2MtPnZ4aWQucHJvY051bWJlcjsKKwkJCQlp
bnN0YW5jZS0+dnhpZC5sb2NhbFRyYW5zYWN0aW9uSWQgPSBwcm9jLT52eGlkLmx4aWQ7CisJ
CQkJaW5zdGFuY2UtPnBpZCA9IHByb2MtPnBpZDsKKwkJCQlpbnN0YW5jZS0+bGVhZGVyUGlk
ID0gcHJvYy0+cGlkOworCQkJCWluc3RhbmNlLT5mYXN0cGF0aCA9IHRydWU7CisKKwkJCQkv
KgorCQkJCSAqIFN1Y2Nlc3NmdWxseSB0YWtpbmcgZmFzdCBwYXRoIGxvY2sgbWVhbnMgdGhl
cmUgd2VyZSBubworCQkJCSAqIGNvbmZsaWN0aW5nIGxvY2tzLgorCQkJCSAqLworCQkJCWlu
c3RhbmNlLT53YWl0U3RhcnQgPSAwOworCisJCQkJZWwrKzsKKwkJCX0KIAkJfQogCiAJCWlm
IChwcm9jLT5mcFZYSURMb2NrKQotLSAKMi40Ni4yCgo=

--------------yGm38uGpVMppwr9IvRHchYXP--