> On 03-Aug-2020, at 8:36 PM, Alvaro Herrera <alvhe...@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 <alvhe...@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