MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_817a8f20dec13726f45d6480eaf38b70"
Content-Transfer-Encoding: 7bit
Date: Wed, 12 Jul 2023 16:52:38 +0900
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Support to define custom wait events for extensions
In-Reply-To: <ZK0IluceH+Hd5h22@paquier.xyz>
References: <b9f5411acda0cf15c8fbb767702ff43e@oss.nttdata.com>
 <CTDCV6CIJAKR.TQOTL86K9628@gonk>
 <e4cef41ee00f791c2264f871dc6b5b19@oss.nttdata.com>
 <CTE7JPUIMMBL.2YLT1IWHPUDZ8@gonk>
 <a9eae9af-3834-ceed-33bc-d36c914fef53@oss.nttdata.com>
 <135feae32cd28f9fb555fe46acb939a0@oss.nttdata.com>
 <f3c10c343e6a6080a6b57f0ed25d9e52@oss.nttdata.com>
 <c18393a428412ede7acd2355c27b3c1f@oss.nttdata.com>
 <ZK0IluceH+Hd5h22@paquier.xyz>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <eaf82a85c0ef1b55dc3b651d3f7b867a@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/eaf82a85c0ef1b55dc3b651d3f7b867a%40oss.nttdata.com>
Precedence: bulk

--=_817a8f20dec13726f45d6480eaf38b70
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=US-ASCII;
 format=flowed

On 2023-07-11 16:45, Michael Paquier wrote:
> On Fri, Jun 23, 2023 at 05:56:26PM +0900, Masahiro Ikeda wrote:
>> I updated the patches to handle the warning mentioned
>> by PostgreSQL Patch Tester, and removed unnecessary spaces.
> 
> I have begun hacking on that, and the API layer inspired from the
> LWLocks is sound.  I have been playing with it in my own extensions
> and it is nice to be able to plug in custom wait events into
> pg_stat_activity, particularly for bgworkers.  Finally.

Great!

> The patch needed a rebase after the recent commit that introduced the
> automatic generation of docs and code for wait events.  It requires
> two tweaks in generate-wait_event_types.pl, feel free to double-check
> them.

Thanks for rebasing. I confirmed it works with the current master.

I know this is a little off-topic from what we're talking about here,
but I'm curious about generate-wait_event_types.pl.

  # generate-wait_event_types.pl
  -	# An exception is required for LWLock and Lock as these don't require
  -	# any C and header files generated.
  +	# An exception is required for Extension, LWLock and Lock as these 
don't
  +	# require any C and header files generated.
   	die "wait event names must start with 'WAIT_EVENT_'"
   	  if ( $trimmedwaiteventname eq $waiteventenumname
  +		&& $waiteventenumname !~ /^Extension/
   		&& $waiteventenumname !~ /^LWLock/
   		&& $waiteventenumname !~ /^Lock/);

In my understanding, the first column of the row for WaitEventExtension 
in
wait_event_names.txt can be any value and the above code should not die.
But if I use the following input, it falls on the last line.

  # wait_event_names.txt
  Section: ClassName - WaitEventExtension

  WAIT_EVENT_EXTENSION	"Extension"	"Waiting in an extension."
  Extension	"Extension"	"Waiting in an extension."
  EXTENSION	"Extension"	"Waiting in an extension."

If the behavior is unexpected, we need to change the current code.
I have created a patch for the areas that I felt needed to be changed.
- 0001-change-the-die-condition-in-generate-wait_event_type.patch
  (In addition to the above, "$continue = ",\n";" doesn't appear to be 
necessary.)


> Some of the new structures and routine names don't quite reflect the
> fact that we have wait events for extensions, so I have taken a stab
> at that.

Sorry. I confirmed the change.


> Note that the test module test_custom_wait_events would crash if
> attempting to launch a worker when not loaded in
> shared_preload_libraries, so we'd better have some protection in
> wait_worker_launch() (this function should be renamed as well).

OK, I will handle it. Since Andres gave me other comments for the
test module, I'll think about what is best.


> Attached is a rebased patch that I have begun tweaking here and
> there.  For now, the patch is moved as waiting on author.  I have
> merged the test module with the main patch for the moment, for
> simplicity.  A split is straight-forward as the code paths touched are
> different.
> 
> Another and *very* important thing is that we are going to require
> some documentation in xfunc.sgml to explain how to use these routines
> and what to expect from them.  Ikeda-san, could you write some?  You
> could look at the part about shmem and LWLock to get some
> inspiration.

OK. Yes, I planned to write documentation.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION
--=_817a8f20dec13726f45d6480eaf38b70
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=0001-change-the-die-condition-in-generate-wait_event_type.patch
Content-Disposition: attachment;
 filename=0001-change-the-die-condition-in-generate-wait_event_type.patch;
 size=1100

RnJvbSA1MTk4ZmM2MzAyYTNiZjQyMzIyNTJiMjNiNDVkMzlkOTg3ZTczNmJjIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXNhaGlybyBJa2VkYSA8bWFzYWhpcm8uaWtlZGEudXNAaGNv
Lm50dC5jby5qcD4KRGF0ZTogV2VkLCAxMiBKdWwgMjAyMyAxNjoyODoyNyArMDkwMApTdWJqZWN0
OiBbUEFUQ0hdIGNoYW5nZSB0aGUgZGllIGNvbmRpdGlvbiBpbiBnZW5lcmF0ZS13YWl0X2V2ZW50
X3R5cGVzLnBsCgotLS0KIHNyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5L2dlbmVyYXRlLXdhaXRf
ZXZlbnRfdHlwZXMucGwgfCA1ICsrLS0tCiAxIGZpbGUgY2hhbmdlZCwgMiBpbnNlcnRpb25zKCsp
LCAzIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5
L2dlbmVyYXRlLXdhaXRfZXZlbnRfdHlwZXMucGwgYi9zcmMvYmFja2VuZC91dGlscy9hY3Rpdml0
eS9nZW5lcmF0ZS13YWl0X2V2ZW50X3R5cGVzLnBsCmluZGV4IDZkMWEyYWY0MmEuLmZiZTAxMTAz
OWEgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5L2dlbmVyYXRlLXdhaXRf
ZXZlbnRfdHlwZXMucGwKKysrIGIvc3JjL2JhY2tlbmQvdXRpbHMvYWN0aXZpdHkvZ2VuZXJhdGUt
d2FpdF9ldmVudF90eXBlcy5wbApAQCAtODksOSArODksOCBAQCBmb3JlYWNoIG15ICRsaW5lIChA
bGluZXNfc29ydGVkKQogCSMgYW55IEMgYW5kIGhlYWRlciBmaWxlcyBnZW5lcmF0ZWQuCiAJZGll
ICJ3YWl0IGV2ZW50IG5hbWVzIG11c3Qgc3RhcnQgd2l0aCAnV0FJVF9FVkVOVF8nIgogCSAgaWYg
KCAkdHJpbW1lZHdhaXRldmVudG5hbWUgZXEgJHdhaXRldmVudGVudW1uYW1lCi0JCSYmICR3YWl0
ZXZlbnRlbnVtbmFtZSAhfiAvXkxXTG9jay8KLQkJJiYgJHdhaXRldmVudGVudW1uYW1lICF+IC9e
TG9jay8pOwotCSRjb250aW51ZSA9ICIsXG4iOworCQkmJiAkd2FpdGNsYXNzbmFtZSAhfiAvXldh
aXRFdmVudExXTG9jayQvCisJCSYmICR3YWl0Y2xhc3NuYW1lICF+IC9eV2FpdEV2ZW50TG9jayQv
KTsKIAlwdXNoKEB7ICRoYXNod2V7JHdhaXRjbGFzc25hbWV9IH0sIEB3YWl0ZXZlbnRsaXN0KTsK
IH0KIAotLSAKMi4yNS4xCgo=
--=_817a8f20dec13726f45d6480eaf38b70--