Incorrect comment regarding command completion tags
I was looking at the code in EndCommand() and noticed a comment
talking about some Asserts which didn't seem to exist in the code.
The comment also talks about LastOid which looks like the name of a
variable that's nowhere to be seen.
It looks like the Asserts did exists when the completion tag patch was
being developed [1]/messages/by-id/1C642743-8E46-4246-B4A0-C9A638B3E88F@enterprisedb.com but they disappeared somewhere along the line and
the comment didn't get an update before 2f9661311 went in.
In the attached, I rewrote the comment to remove mention of the
Asserts. I also tried to form the comment in a way that's more
understandable about why we always write a "0" in "INSERT 0 <nrows>".
David
[1]: /messages/by-id/1C642743-8E46-4246-B4A0-C9A638B3E88F@enterprisedb.com
Attachments:
fix_completion_tag_comment.patchtext/plain; charset=US-ASCII; name=fix_completion_tag_comment.patchDownload
diff --git a/src/backend/tcop/dest.c b/src/backend/tcop/dest.c
index c952cbea8b..e6934d7b66 100644
--- a/src/backend/tcop/dest.c
+++ b/src/backend/tcop/dest.c
@@ -179,12 +179,11 @@ EndCommand(const QueryCompletion *qc, CommandDest dest, bool force_undecorated_o
* We assume the tagname is plain ASCII and therefore requires no
* encoding conversion.
*
- * We no longer display LastOid, but to preserve the wire
- * protocol, we write InvalidOid where the LastOid used to be
- * written.
- *
- * All cases where LastOid was written also write nprocessed
- * count, so just Assert that rather than having an extra test.
+ * In ancient versions of PostgreSQL, INSERT used to include the
+ * Oid of the inserted record in the completion tag. We no longer
+ * support tables with Oids, so to maintain compatibility in the
+ * wire protocol, we write a "0" for InvalidOid in the location
+ * where we once wrote the inserted record's Oid.
*/
tag = qc->commandTag;
tagname = GetCommandTagName(tag);
On Oct 13, 2022, at 2:56 PM, David Rowley <dgrowleyml@gmail.com> wrote:
I was looking at the code in EndCommand() and noticed a comment
talking about some Asserts which didn't seem to exist in the code.
The comment also talks about LastOid which looks like the name of a
variable that's nowhere to be seen.It looks like the Asserts did exists when the completion tag patch was
being developed [1] but they disappeared somewhere along the line and
the comment didn't get an update before 2f9661311 went in.In the attached, I rewrote the comment to remove mention of the
Asserts. I also tried to form the comment in a way that's more
understandable about why we always write a "0" in "INSERT 0 <nrows>".
Your wording is better. +1
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, 14 Oct 2022 at 11:38, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
On Oct 13, 2022, at 2:56 PM, David Rowley <dgrowleyml@gmail.com> wrote:
In the attached, I rewrote the comment to remove mention of the
Asserts. I also tried to form the comment in a way that's more
understandable about why we always write a "0" in "INSERT 0 <nrows>".Your wording is better. +1
Thanks for having a look. I adjusted the wording slightly as I had
written "ancient" in regards to PostgreSQL 11 and earlier. It's
probably a bit early to call a supported version of PostgreSQL ancient
so I just decided to mention the version number instead.
I pushed the resulting patch.
Thanks for having a look.
David