public inbox for [email protected]  

help / color / mirror / Atom feed

From: Heikki Linnakangas <[email protected]>
To: Jacob Champion <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: pgsql: Make cancel request keys longer
Date: Fri, 9 May 2025 09:37:58 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAOYmi+kJcKkbe3Ng7=jYOz7V2gYEvdNXKESKbmuW=6N7=243tg@mail.gmail.com>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CAOYmi+kJcKkbe3Ng7=jYOz7V2gYEvdNXKESKbmuW=6N7=243tg@mail.gmail.com>

On 09/05/2025 01:28, Jacob Champion wrote:
> On Thu, May 8, 2025 at 12:11 PM Heikki Linnakangas <[email protected]> wrote:
>> Polished this up a tiny bit, and committed.
> 
> Thanks! I think the uint8->int change for cancel_key_len is more than
> just cosmetic; it most likely fixes a bug where a key size of 256
> wrapped around to 0. I'll double-check that this fixes that later;
> I've gotten side-tracked from the protocol stuff a bit.

True, although I'm pretty sure you'd fail the later cross-check that the 
whole message was consumed. ("message contents do not agree with length 
in message type"). But it's fixed now in any case.

> While I have you, though, is the following just a really complicated
> way to say `msgLength - 4`, or is there some other reason to do the
> pointer math?
> 
>      cancel_key_len = 5 + msgLength - (conn->inCursor - conn->inStart);

Yes, it amounts to 'msgLength - 4'. I agree it looks pretty obscure. The 
way to read it is:

/* full length of the message, including the type code byte and the 
length field itself */
fullMsgLength = 5 + msgLength;

/* number of bytes consumed from the message so far */
lengthConsumed = (conn->inCursor - conn->inStart);

/* the cancel key consumes all the remaining bytes of the message */
cancel_key_len = fullMsgLength - lengthConsumed;

It didn't occur to me that you could write it simply as 'msgLength - 4'. 
That depends on knowing that the preceding fields are exactly 4 bytes 
long, but that's clear enough if we just add a comment on that, see 
attached.

-- 
Heikki Linnakangas
Neon (https://neon.tech)


Attachments:

  [text/x-patch] simplify-cancel_key_len-math.patch (632B, 2-simplify-cancel_key_len-math.patch)
  download | inline diff:
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index beb1c889aad..551276e9011 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -1544,7 +1544,11 @@ getBackendKeyData(PGconn *conn, int msgLength)
 	if (pqGetInt(&(conn->be_pid), 4, conn))
 		return EOF;
 
-	cancel_key_len = 5 + msgLength - (conn->inCursor - conn->inStart);
+	/*
+	 * The pid consumed 4 bytes, and the cancellation key consumes the rest of
+	 * the message.
+	 */
+	cancel_key_len = msgLength - 4;
 
 	conn->be_cancel_key = malloc(cancel_key_len);
 	if (conn->be_cancel_key == NULL)

view thread (11+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: pgsql: Make cancel request keys longer
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox