[PATCH] - Provide robust alternatives for replace_string

What happens if a replacement string happens to be split in the middle
by the fgets buffering? I think it'll fail to be replaced. This
applies to both versions.

In the stringinfo version it seemed to me that using pnstrdup is
possible to avoid copying trailing bytes.

If you're asking for opinion, mine is that StringInfo looks to be the
better approach, and also you don't need to keep API compatibility.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Thank you Alvaro for reviewing the patch!

On 01-Aug-2020, at 7:22 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

What happens if a replacement string happens to be split in the middle
by the fgets buffering? I think it'll fail to be replaced. This
applies to both versions.

Can a string to be replaced be split across multiple lines in the source file? If I understand correctly, fgets reads one line from input file at a time. If I do not, in the worst case, we will get an un-replaced string in the output, such as “@abs_dir@“ and it should be easily detected by a failing diff.

In the stringinfo version it seemed to me that using pnstrdup is
possible to avoid copying trailing bytes.

That’s a good suggestion. Using pnstrdup would look like this:

--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -465,7 +465,7 @@ replace_stringInfo(StringInfo string, const char *replace, const char *replaceme

        while ((ptr = strstr(string->data, replace)) != NULL)
        {
-               char       *dup = pg_strdup(string->data);
+              char       *dup = pnstrdup(string->data, string->maxlen);
                size_t          pos = ptr - string->data;

string->len = pos;

If you're asking for opinion, mine is that StringInfo looks to be the
better approach, and also you don't need to keep API compatibility.

Thank you. We also prefer StringInfo solution.

Asim

On 2020-Aug-03, Asim Praveen wrote:

Thank you Alvaro for reviewing the patch!

On 01-Aug-2020, at 7:22 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

What happens if a replacement string happens to be split in the middle
by the fgets buffering? I think it'll fail to be replaced. This
applies to both versions.

Can a string to be replaced be split across multiple lines in the source file? If I understand correctly, fgets reads one line from input file at a time. If I do not, in the worst case, we will get an un-replaced string in the output, such as “@abs_dir@“ and it should be easily detected by a failing diff.

I meant what if the line is longer than 1023 chars and the replace
marker starts at byte 1021, for example. Then the first fgets would get
"@ab" and the second fgets would get "s_dir@" and none would see it as
replaceable.

In the stringinfo version it seemed to me that using pnstrdup is
possible to avoid copying trailing bytes.

That’s a good suggestion. Using pnstrdup would look like this:

--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -465,7 +465,7 @@ replace_stringInfo(StringInfo string, const char *replace, const char *replaceme

while ((ptr = strstr(string->data, replace)) != NULL)
{
-               char       *dup = pg_strdup(string->data);
+              char       *dup = pnstrdup(string->data, string->maxlen);

I was thinking pnstrdup(string->data, ptr - string->data) to avoid
copying the chars beyond ptr.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 03-Aug-2020, at 8:36 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2020-Aug-03, Asim Praveen wrote:

Thank you Alvaro for reviewing the patch!

On 01-Aug-2020, at 7:22 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

What happens if a replacement string happens to be split in the middle
by the fgets buffering? I think it'll fail to be replaced. This
applies to both versions.

Can a string to be replaced be split across multiple lines in the source file? If I understand correctly, fgets reads one line from input file at a time. If I do not, in the worst case, we will get an un-replaced string in the output, such as “@abs_dir@“ and it should be easily detected by a failing diff.

I meant what if the line is longer than 1023 chars and the replace
marker starts at byte 1021, for example. Then the first fgets would get
"@ab" and the second fgets would get "s_dir@" and none would see it as
replaceable.

Thanks for the patient explanation, I had missed the obvious. To keep the code simple, I’m in favour of relying on the diff of a failing test to catch the split-replacement string problem.

In the stringinfo version it seemed to me that using pnstrdup is
possible to avoid copying trailing bytes.

That’s a good suggestion. Using pnstrdup would look like this:

--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -465,7 +465,7 @@ replace_stringInfo(StringInfo string, const char *replace, const char *replaceme

while ((ptr = strstr(string->data, replace)) != NULL)
{
-               char       *dup = pg_strdup(string->data);
+              char       *dup = pnstrdup(string->data, string->maxlen);

I was thinking pnstrdup(string->data, ptr - string->data) to avoid
copying the chars beyond ptr.

In fact, what we need in the dup are chars beyond ptr. Copying of characters prefixing the string to be replaced can be avoided, like so:

--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -465,12 +465,12 @@ replace_stringInfo(StringInfo string, const char *replace, const char *replaceme

        while ((ptr = strstr(string->data, replace)) != NULL)
        {
-               char       *dup = pg_strdup(string->data);
+               char       *suffix = pnstrdup(ptr + strlen(replace), string->maxlen);
                size_t          pos = ptr - string->data;

                string->len = pos;
                appendStringInfoString(string, replacement);
-               appendStringInfoString(string, dup + pos + strlen(replace));
+               appendStringInfoString(string, suffix);

-               free(dup);
+               free(suffix);
        }
}

Asim

On 03-Aug-2020, at 8:36 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2020-Aug-03, Asim Praveen wrote:

Thank you Alvaro for reviewing the patch!

On 01-Aug-2020, at 7:22 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

What happens if a replacement string happens to be split in the middle
by the fgets buffering? I think it'll fail to be replaced. This
applies to both versions.

Can a string to be replaced be split across multiple lines in the source file? If I understand correctly, fgets reads one line from input file at a time. If I do not, in the worst case, we will get an un-replaced string in the output, such as “@abs_dir@“ and it should be easily detected by a failing diff.

I meant what if the line is longer than 1023 chars and the replace
marker starts at byte 1021, for example. Then the first fgets would get
"@ab" and the second fgets would get "s_dir@" and none would see it as
replaceable.

Please find attached a StringInfo based solution to this problem. It uses fgetln instead of fgets such that a line is read in full, without ever splitting it.

Asim

On 2020-Aug-05, Asim Praveen wrote:

Please find attached a StringInfo based solution to this problem. It
uses fgetln instead of fgets such that a line is read in full, without
ever splitting it.

never heard of fgetln, my system doesn't have a manpage for it, and we
don't use it anywhere AFAICS. Are you planning to add something to
src/common for it?

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 05-Aug-2020, at 7:01 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2020-Aug-05, Asim Praveen wrote:

Please find attached a StringInfo based solution to this problem. It
uses fgetln instead of fgets such that a line is read in full, without
ever splitting it.

never heard of fgetln, my system doesn't have a manpage for it, and we
don't use it anywhere AFAICS. Are you planning to add something to
src/common for it?

Indeed! I noticed fgetln on the man page of fgets and used it without checking. And this happened on a MacOS system.

Please find a revised version that uses fgetc instead.

Asim

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, 7 August 2020 09:02, Asim Praveen <pasim@vmware.com> wrote:

On 05-Aug-2020, at 7:01 PM, Alvaro Herrera alvherre@2ndquadrant.com wrote:
On 2020-Aug-05, Asim Praveen wrote:

Please find attached a StringInfo based solution to this problem. It
uses fgetln instead of fgets such that a line is read in full, without
ever splitting it.

never heard of fgetln, my system doesn't have a manpage for it, and we
don't use it anywhere AFAICS. Are you planning to add something to
src/common for it?

Indeed! I noticed fgetln on the man page of fgets and used it without checking. And this happened on a MacOS system.

Please find a revised version that uses fgetc instead.

Although not an issue in the current branch, fgetc might become a bit slow
in large files. Please find v3 which simply continues reading the line if
fgets fills the buffer and there is still data to read.

Also this version, implements Alvaro's suggestion to break API compatibility.

To that extent, ecpg regress has been slightly modified to use the new version
of replace_string() where needed, or remove it all together where possible.

//Georgios

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, 19 August 2020 11:07, Georgios <gkokolatos@protonmail.com> wrote:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, 7 August 2020 09:02, Asim Praveen pasim@vmware.com wrote:

On 05-Aug-2020, at 7:01 PM, Alvaro Herrera alvherre@2ndquadrant.com wrote:
On 2020-Aug-05, Asim Praveen wrote:

Please find attached a StringInfo based solution to this problem. It
uses fgetln instead of fgets such that a line is read in full, without
ever splitting it.

never heard of fgetln, my system doesn't have a manpage for it, and we
don't use it anywhere AFAICS. Are you planning to add something to
src/common for it?

Indeed! I noticed fgetln on the man page of fgets and used it without checking. And this happened on a MacOS system.
Please find a revised version that uses fgetc instead.

Although not an issue in the current branch, fgetc might become a bit slow
in large files. Please find v3 which simply continues reading the line if
fgets fills the buffer and there is still data to read.

Also this version, implements Alvaro's suggestion to break API compatibility.

To that extent, ecpg regress has been slightly modified to use the new version
of replace_string() where needed, or remove it all together where possible.

I noticed that the cfbot [1]https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.105985 was unhappy with the raw use of __attribute__ on windows builds.

In retrospect it is rather obvious it would complain. Please find v4 attached.

//Georgios

//Georgios

Asim

[1]: https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.105985

Note that starting with commit 67a472d71c98 you can use pg_get_line and
not worry about the hard part of this anymore :-)

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Note that starting with commit 67a472d71c98 you can use pg_get_line and
not worry about the hard part of this anymore :-)

pg_get_line as it stands isn't quite suitable, because it just hands
back a "char *" string, not a StringInfo that you can do further
processing on.

However, I'd already grown a bit dissatisfied with exposing only that
API, because the code 8f8154a50 added to hba.c couldn't use pg_get_line
either, and had to duplicate the logic. So the attached revised patch
splits pg_get_line into two pieces, one with the existing char * API
and one that appends to a caller-provided StringInfo. (hba.c needs the
append-rather-than-reset behavior, and it might be useful elsewhere
too.)

While here, I couldn't resist getting rid of ecpg_filter()'s hard-wired
line length limit too.

This version looks committable to me, though perhaps someone has
further thoughts?

regards, tom lane

I wrote:

This version looks committable to me, though perhaps someone has
further thoughts?

I looked through this again and pushed it.

regards, tom lane