[PATCH] Fix escaping for '\' and '"' in pageinspect for gist
Hi hackers!
I noticed, that there is bug in escaping values that contains '\' or '"' in text representation
inside pageinspect for gist: the string 'foo"bar' are printed like "foo""bar" and not "foo\"bar".
To fix that, we should do appendStringInfoCharMacro(&buf, '\\'); instead of
appendStringInfoCharMacro(&buf, ch); in case ch is one of that symbols.
Any thoughts?
--
Best regards,
Roman Khapov
Attachments:
v1-0001-fix-escaping-in-pageinspect-for-gist.patchapplication/octet-stream; name=v1-0001-fix-escaping-in-pageinspect-for-gist.patch; x-unix-mode=0644Download
From 8d5623ec54f8ab304b1061179eae372a48fb696e Mon Sep 17 00:00:00 2001
From: roman khapov <r.khapov@ya.ru>
Date: Mon, 29 Dec 2025 15:36:25 +0000
Subject: [PATCH v1] fix escaping in pageinspect for gist
Characters '\' and '"' was written twice but not escaped
Author: Roman Khapov <r.khapov@ya.ru>
---
contrib/pageinspect/gistfuncs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/pageinspect/gistfuncs.c b/contrib/pageinspect/gistfuncs.c
index 414513c395b..6805597b99f 100644
--- a/contrib/pageinspect/gistfuncs.c
+++ b/contrib/pageinspect/gistfuncs.c
@@ -340,7 +340,7 @@ gist_page_items(PG_FUNCTION_ARGS)
char ch = *tmp;
if (ch == '"' || ch == '\\')
- appendStringInfoCharMacro(&buf, ch);
+ appendStringInfoCharMacro(&buf, '\\');
appendStringInfoCharMacro(&buf, ch);
}
if (nq)
--
2.43.0
On Mon, 29 Dec 2025 at 20:48, Roman Khapov <rkhapov@yandex-team.ru> wrote:
Hi hackers!
I noticed, that there is bug in escaping values that contains '\' or '"' in text representation
inside pageinspect for gist: the string 'foo"bar' are printed like "foo""bar" and not "foo\"bar".To fix that, we should do appendStringInfoCharMacro(&buf, '\\'); instead of
appendStringInfoCharMacro(&buf, ch); in case ch is one of that symbols.Any thoughts?
--
Best regards,
Roman Khapov
Hi!
Looks like we have two similar places in kernel [0]https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/rowtypes.c#L460 & [1]https://github.com/postgres/postgres/blob/master/src/bin/pg_rewind/libpq_source.c#L621 -- Best regards, Kirill Reshke, and at
least one of these three places is wrong.
[0]: https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/rowtypes.c#L460
[1]: https://github.com/postgres/postgres/blob/master/src/bin/pg_rewind/libpq_source.c#L621 -- Best regards, Kirill Reshke
--
Best regards,
Kirill Reshke
Roman Khapov <rkhapov@yandex-team.ru> writes:
I noticed, that there is bug in escaping values that contains '\' or '"' in text representation
inside pageinspect for gist: the string 'foo"bar' are printed like "foo""bar" and not "foo\"bar".
I do not think this is a bug. The comment at line 295 says
"Most of this is copied from record_out().", and this logic
matches what record_out() does, and the output is legal
according to the manual's specifications [1]https://www.postgresql.org/docs/devel/rowtypes.html#ROWTYPES-IO-SYNTAX:
To put a double quote or backslash in a quoted composite field
value, precede it with a backslash. (Also, a pair of double quotes
within a double-quoted field value is taken to represent a double
quote character, analogously to the rules for single quotes in SQL
literal strings.)
Now, your alternative coding would also produce legal output, but
I do not think unnecessary change here is a good thing.
regards, tom lane
[1]: https://www.postgresql.org/docs/devel/rowtypes.html#ROWTYPES-IO-SYNTAX
On Mon, 29 Dec 2025 at 23:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Roman Khapov <rkhapov@yandex-team.ru> writes:
I noticed, that there is bug in escaping values that contains '\' or '"' in text representation
inside pageinspect for gist: the string 'foo"bar' are printed like "foo""bar" and not "foo\"bar".I do not think this is a bug. The comment at line 295 says
"Most of this is copied from record_out().", and this logic
matches what record_out() does, and the output is legal
according to the manual's specifications [1]:To put a double quote or backslash in a quoted composite field
value, precede it with a backslash. (Also, a pair of double quotes
within a double-quoted field value is taken to represent a double
quote character, analogously to the rules for single quotes in SQL
literal strings.)Now, your alternative coding would also produce legal output, but
I do not think unnecessary change here is a good thing.regards, tom lane
[1] https://www.postgresql.org/docs/devel/rowtypes.html#ROWTYPES-IO-SYNTAX
Should we then refactor code to avoid copying? I copied this code in
[0]: /messages/by-id/CALdSSPgpD5RfPn5qMbozU4_SQpZAbG3V_=KdxV9YaEG9gX=qEA@mail.gmail.com
will be 3 times copied code...
[0]: /messages/by-id/CALdSSPgpD5RfPn5qMbozU4_SQpZAbG3V_=KdxV9YaEG9gX=qEA@mail.gmail.com
--
Best regards,
Kirill Reshke
On Tue, 30 Dec 2025 at 00:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Kirill Reshke <reshkekirill@gmail.com> writes:
Should we then refactor code to avoid copying? I copied this code in
[0] for the third time, so if this has a chance to be committed, there
will be 3 times copied code...Might not be a bad idea, but of course the devil is in the details.
What did you have in mind? Split out a function to quote one
field value per record_out's rules?regards, tom lane
Yes, split out a function that does quoting after
OidOutputFunctionCall. that is, this:
https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/rowtypes.c#L437-L464
--
Best regards,
Kirill Reshke
Import Notes
Reply to msg id not found: 2993271.1767036289@sss.pgh.pa.us
On Tue, Dec 30, 2025 at 12:29:49AM +0500, Kirill Reshke wrote:
Yes, split out a function that does quoting after
OidOutputFunctionCall. that is, this:https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/rowtypes.c#L437-L464
FWIW, refactoring that into a single function is something I have
considered while working on e7bff46e50b8, as something that could be
done on HEAD. At the end, I could not get excited with the result I
got (see the end of the thread) for the amount of lines saved, also
because this impacts a limited area of the code with what seems like a
reduction in readability.
--
Michael