Thread: [PATCH] Make pg_basebackup configure and start standby [Review]

[PATCH] Make pg_basebackup configure and start standby [Review]

From

Amit Kapila

Date:

20 September 2012, 13:31:46

<div class="WordSection1"><p class="MsoNormal"><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black">On</span><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black">Sun,01 Jul 2012 13:02:17 +0200 Boszormenyi Zoltan wrote:</span><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black"></span><p class="MsoNormal"><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black">>attached is a patch that does
$SUBJECT.</span><pclass="MsoNormal"><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black"> </span><pclass="MsoNormal"><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black">>It'sa usability enhancement, to take a backup, write</span><p class="MsoNormal"><span
lang="EN"style="font-size:12.0pt;font-family:"Times New Roman","serif";color:black">>a minimalistic recovery.conf
andstart the streaming</span><p class="MsoNormal"><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black">>standbyin one go.</span><p class="MsoNormal"><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black"> </span><p class="MsoNormal"><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black">>Comments?</span><p
class="MsoNormal"><spanlang="EN" style="font-size:12.0pt;font-family:"Times New Roman","serif";color:black"> </span><p
class="MsoNormal"><b><spanlang="EN" style="font-size:12.0pt;font-family:"Times New Roman","serif";color:black">[Review
ofPatch]</span></b><p class="MsoNormal"><span lang="EN" style="font-size:12.0pt;font-family:"Times New
Roman","serif";color:black"> </span><pclass="MsoNormal"><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Basicstuff:</span></b><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">----------------------</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-Patch applies OK </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-Compiles cleanly with no warnings</span><br /><br /><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Whatit does:</span></b><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-------------------------</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Thepg_basebackup tool does the backup of Cluster from server
tothe specified location. </span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">This new
functionalitywill also writes the recovery.conf in the database directory and start the standby server based on options
passedto pg_basebackup.</span><br /><br /><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Usability</span></b><br/><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">----------------</span></b><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Forusability aspect, I am not very sure how many users would
liketo start the standby server using basebackup. </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Accordingto me it can be useful for users who have automated
scriptsto start server after backup can use this feature.</span><br /><br /><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">FeatureTesting:</span></b><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-----------------------------</span><br/><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">1.Test pg_basebackup with option -R to check that the
recovery.conffile is written to data directory.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   --recovery.conf file is created in data
directory.</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">2.Test pg_basebackup with option -R to check that the
recovery.conffile is not able to create because of disk full.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   --Error is given as recovery.conf file is not able to
create.</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">     </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">3.Test pg_basebackup with option -S to check the standby
serverstart on the same/different machine.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   --Starting standby server is success in if pg_basebackup
istaken in different machine.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   
</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">4. Test pg_basebackup with both options -S
and-R to check the standby server start on same/different machine.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   --Starting standby server is success in if pg_basebackup
istaken in different machine.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   
</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">5. Test pg_basebackup with option -S
including-h, -U, -p, -w and -W to check the standy server start </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  and verify the recovery.conf which is created in data
directory.</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    --Except password, rest of
theprimary connection info parameters are working fine.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">6.Test pg_basebackup with conflict options (-x or -X and -R
or-S).</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    --Error is given when the
conflictoptions are provided to pg_basebackup.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">7.Test pg_basebackup with option -S where pg_ctl/postmaster
binariesare not present in the path.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   
--Erroris given as not able to execute.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   
</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">8. Test pg_basebackup with option -S by
connectingto a standby server.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   
--standbyserver is started successfully when pg_basebackup is made from a standby server also.</span><br /><br
/><b><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">Code Review:</span></b><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">----------------------------</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">1.In function WriteRecoveryConf(), un-initialized filename is
used.</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    due to which it can print junk for
belowline in code</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">   printf("add password
toprimary_conninfo in %s if needed\n", filename);</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">2. In function WriteRecoveryConf(), in below code if fopen
fails(due to disk full or any other file related error) it will print the error and exits.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">    So now it can be confusing to user, in respect to can he
considerbackup as successfull and proceed. IMO, either error meesage or documentation</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">    can suggest the for such error user can proceed with
backupto write his own recovery.conf and start the standby.</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">+       cf = fopen(filename, "w");</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">+       if (cf == NULL)</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">+       {</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">+               fprintf(stderr, _("cannot create %s"),
filename);</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">+              
 exit(1);</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">+        }</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">3.In function main,</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   instead of the following code it can be changed in two
differentways,</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">           if (startstandby)</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                   writerecoveryconf = true;</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">    </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   change1:</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">       case 'R':</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       writerecoveryconf = true;</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">                        break;</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">               case 'S':</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       startstandby = true;</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       writerecoveryconf = true;</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">                        break;</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   change2:</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">               case 'S':</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       startstandby = true;</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">       case 'R':</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">                       writerecoveryconf = true;</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">                        break;</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">4.The password is not written to primary_conninfo even if the
dbpasswordis present because of this reason</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  connecting to the primary is failing because of
authenticationfailure.</span><br /><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">5. write the
functionheader for the newly added functions.</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">6.execvp function is deprecated beginning in Visual C++ 2005.
whichis used to fork the pg_ctl process.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif""> 
 <a
href="http://msdn.microsoft.com/en-us/library/ms235414.aspx">http://msdn.microsoft.com/en-us/library/ms235414.aspx</a></span><br
/><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">7. In StartStandby function, it is better to
freethe memory allocated for path (path = xstrdup(command);) </span><br /><br /><br /><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Defects:</span></b><br/><b><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-------------</span></b><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">1.If the pg_basebackup is used in the same machine with the
optionof -S, the standby server start</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif""> 
 willfail as the port already in use because of using the same postgresql.conf.</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">2.If the hot_standby=off in master conf file, the same is
copiedto subscriber and starts the server. with that</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">  no client connections are allowed to the server.</span><br
/><br/><b><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">Documentation issues:</span></b><br
/><b><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">--------------------------------</span></b><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">1. For -R option, </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Conflictswith <option>--xlog</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Ithink it is better to explain the reason of
conflict.</span><br/><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">2. For -S option, </span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">    "Start the standby database server. Implies -R
option."</span><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    I think the above can be
improvedto </span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    "Writes the recovery.conf
andstart the standby database server. There is no need for user to specify -R option explicitly."</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   or something similar.</span><br /><br /><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black"></span><p class="MsoNormal"><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black"> </span><p class="MsoNormal"><span lang="EN"
style="font-size:12.0pt;font-family:"TimesNew Roman","serif";color:black">With Regards,</span><p
class="MsoNormal"><spanlang="EN" style="font-size:12.0pt;font-family:"Times New Roman","serif";color:black">Amit
Kapila.</span><pclass="MsoNormal"><span lang="EN" style="font-size:13.0pt;font-family:"Courier
New";color:black"> </span><pclass="MsoNormal"> </div>

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

03 October 2012, 19:17:08

Hi,

first, thanks for the review. Comments are below.

2012-09-20 12:30 keltezéssel, Amit Kapila írta:

On Sun, 01 Jul 2012 13:02:17 +0200 Boszormenyi Zoltan wrote:
>attached is a patch that does $SUBJECT.

>It's a usability enhancement, to take a backup, write
>a minimalistic recovery.conf and start the streaming
>standby in one go.

>Comments?

[Review of Patch]

Basic stuff:
----------------------
- Patch applies OK

This is not true anymore with a newer GIT version.
Some chunk for pg_basebackup.c was rejected.

- Compiles cleanly with no warnings

What it does:
-------------------------
The pg_basebackup tool does the backup of Cluster from server to the specified location.
This new functionality will also writes the recovery.conf in the database directory and start the standby server based on options passed to pg_basebackup.

Usability
----------------
For usability aspect, I am not very sure how many users would like to start the standby server using basebackup.

Also, Magnus raised the point that it wouldn't really work on MS Windows
where you *have to* start the service via OS facilities. This part of the patch
was killed.

According to me it can be useful for users who have automated scripts to start server after backup can use this feature.

Well, scripting is not portable across UNIXes and Windows,
you have to spell out starting the server differently.

Feature Testing:
-----------------------------

1. Test pg_basebackup with option -R to check that the recovery.conf file is written to data directory.
--recovery.conf file is created in data directory.

2. Test pg_basebackup with option -R to check that the recovery.conf file is not able to create because of disk full.
--Error is given as recovery.conf file is not able to create.

3. Test pg_basebackup with option -S to check the standby server start on the same/different machine.
--Starting standby server is success in if pg_basebackup is taken in different machine.

4. Test pg_basebackup with both options -S and -R to check the standby server start on same/different machine.
--Starting standby server is success in if pg_basebackup is taken in different machine.

5. Test pg_basebackup with option -S including -h, -U, -p, -w and -W to check the standy server start
and verify the recovery.conf which is created in data directory.
--Except password, rest of the primary connection info parameters are working fine.

The password part is now fixed.

6. Test pg_basebackup with conflict options (-x or -X and -R or -S).
--Error is given when the conflict options are provided to pg_basebackup.

7. Test pg_basebackup with option -S where pg_ctl/postmaster binaries are not present in the path.
--Error is given as not able to execute.

8. Test pg_basebackup with option -S by connecting to a standby server.
--standby server is started successfully when pg_basebackup is made from a standby server also.

Code Review:
----------------------------
1. In function WriteRecoveryConf(), un-initialized filename is used.
due to which it can print junk for below line in code
printf("add password to primary_conninfo in %s if needed\n", filename);

Fixed.

2. In function WriteRecoveryConf(), in below code if fopen fails (due to disk full or any other file related error) it will print the error and exits.
So now it can be confusing to user, in respect to can he consider backup as successfull and proceed. IMO, either error meesage or documentation
can suggest the for such error user can proceed with backup to write his own recovery.conf and start the standby.

+ cf = fopen(filename, "w");
+ if (cf == NULL)
+ {
+ fprintf(stderr, _("cannot create %s"), filename);
+ exit(1);
+ }

But BaseBackup() function did indicate already that it has
finished successfully with

if (verbose)
fprintf(stderr, "%s: base backup completed\n", progname);

Would it be an expected (as in: not confusing) behaviour to return 0
from pg_basebackup if the backup itself has finished, but failed to write
the recovery.conf or start the standby if those were requested?

I have modified my WriteRecoveryConf() to do exit(2) instead of exit(1)
to indicate a different error. exit(1) seems to be for reporting configuration
or connection errors. (I may be mistaken though.)

3. In function main,
instead of the following code it can be changed in two different ways,

if (startstandby)
writerecoveryconf = true;

change1:
case 'R':
writerecoveryconf = true;
break;
case 'S':
startstandby = true;
writerecoveryconf = true;
break;

change2:
case 'S':
startstandby = true;
case 'R':
writerecoveryconf = true;
break;

I went with your second variant at first but it's not needed anymore
as only "-R" exists.

4. The password is not written to primary_conninfo even if the dbpassword is present because of this reason
connecting to the primary is failing because of authentication failure.

Fixed.

5. write the function header for the newly added functions.

Fixed.

6. execvp function is deprecated beginning in Visual C++ 2005. which is used to fork the pg_ctl process.
http://msdn.microsoft.com/en-us/library/ms235414.aspx

This issue is now irrelevant as the standby is not started, there is no "-S" option.

7. In StartStandby function, it is better to free the memory allocated for path (path = xstrdup(command);)

Same as above.

Defects:
-------------
1. If the pg_basebackup is used in the same machine with the option of -S, the standby server start
will fail as the port already in use because of using the same postgresql.conf.

Well, running initdb twice on the same machine with different data directories
would also cause the second server fail to start because of the same issue
and it's not called a bug. I think this is irrelevant as is and also because there
is no "-S" now.

2. If the hot_standby=off in master conf file, the same is copied to subscriber and starts the server. with that
no client connections are allowed to the server.

Well, it simply copies the source server behaviour, which can also be a
replication standby. PostgreSQL has cascading replication, you know.

Documentation issues:
--------------------------------
1. For -R option,
Conflicts with <option>--xlog
I think it is better to explain the reason of conflict.

Fixed.

2. For -S option,
"Start the standby database server. Implies -R option."
I think the above can be improved to
"Writes the recovery.conf and start the standby database server. There is no need for user to specify -R option explicitly."
or something similar.

Not relevant anymore.

Again, thanks for the review.

The second generation of this work is now attached and contains a new
feature as was discussed and suggested by Magnus Hagander, Fujii Masao
and Peter Eisentraut. So libpq has grown a new function:

+/* return the connection options used by a live connection */
+extern PQconninfoOption *PQconninfo(PGconn *conn);

This copies all the connection parameters back from the live PGconn itself
so everything that's needed to connect is already validated.

This is used by the second patch which makes the changes in pg_basebackup
simpler and not hardcoded.

Please, review.

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de    http://www.postgresql.at/

2012-10-04 06:47 keltezéssel, Boszormenyi Zoltan írta:
> 2012-10-04 05:24 keltezéssel, Peter Eisentraut írta:
>> On Wed, 2012-10-03 at 18:15 +0200, Boszormenyi Zoltan wrote:
>>> The second generation of this work is now attached and contains a new
>>> feature as was discussed and suggested by Magnus Hagander, Fujii Masao
>>> and Peter Eisentraut. So libpq has grown a new function:
>>>
>>> +/* return the connection options used by a live connection */
>>> +extern PQconninfoOption *PQconninfo(PGconn *conn);
>>>
>>> This copies all the connection parameters back from the live PGconn
>>> itself
>>> so everything that's needed to connect is already validated.
>> I don't like that this code maintains a second list of all possible
>> libpq connection parameters.
>
> Where does it do that? In PQconninfo() itself? Why is it a problem?
> Or to put it bluntly: the same problem is in fillPGconn() too, as it also
> has the same set of parameters listed. So there is already code
> that you don't like. :-)
>
> How about a static mapping between option names and
> offsetof(struct pg_conn, member) values? This way both fillPGconn()
> and PQconninfo() can avoid maintaining the list of parameter names.

Did you think about something like the attached code?

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

Attachment

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

04 October 2012, 14:18:30

2012-10-04 12:42 keltezéssel, Boszormenyi Zoltan írta:
> 2012-10-04 06:47 keltezéssel, Boszormenyi Zoltan írta:
>> 2012-10-04 05:24 keltezéssel, Peter Eisentraut írta:
>>> On Wed, 2012-10-03 at 18:15 +0200, Boszormenyi Zoltan wrote:
>>>> The second generation of this work is now attached and contains a new
>>>> feature as was discussed and suggested by Magnus Hagander, Fujii Masao
>>>> and Peter Eisentraut. So libpq has grown a new function:
>>>>
>>>> +/* return the connection options used by a live connection */
>>>> +extern PQconninfoOption *PQconninfo(PGconn *conn);
>>>>
>>>> This copies all the connection parameters back from the live PGconn
>>>> itself
>>>> so everything that's needed to connect is already validated.
>>> I don't like that this code maintains a second list of all possible
>>> libpq connection parameters.
>>
>> Where does it do that? In PQconninfo() itself? Why is it a problem?
>> Or to put it bluntly: the same problem is in fillPGconn() too, as it also
>> has the same set of parameters listed. So there is already code
>> that you don't like. :-)
>>
>> How about a static mapping between option names and
>> offsetof(struct pg_conn, member) values? This way both fillPGconn()
>> and PQconninfo() can avoid maintaining the list of parameter names.
>
> Did you think about something like the attached code?

Or rather this one, which fixes a bug so fillPGconn() and PQconninfo() are
symmetric and work for "requiressl".

>
> Best regards,
> Zoltán Böszörményi



--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

Attachment

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Tom Lane

Date:

04 October 2012, 17:43:23

Boszormenyi Zoltan <zb@cybertec.at> writes:
>> Did you think about something like the attached code?

> Or rather this one, which fixes a bug so fillPGconn() and PQconninfo() are
> symmetric and work for "requiressl".

That's incredibly ugly.  I'm not sure where we should keep the "R"
information, but shoehorning it into the existing PQconninfoOption
struct like that seems totally unacceptable.  Either we're willing to
break backwards compatibility on the Disp-Char strings, or we need
to put that info somewhere else.
        regards, tom lane

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

04 October 2012, 19:20:13

2012-10-04 16:43 keltezéssel, Tom Lane írta:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
>>> Did you think about something like the attached code?
>> Or rather this one, which fixes a bug so fillPGconn() and PQconninfo() are
>> symmetric and work for "requiressl".
> That's incredibly ugly.  I'm not sure where we should keep the "R"
> information, but shoehorning it into the existing PQconninfoOption
> struct like that seems totally unacceptable.  Either we're willing to
> break backwards compatibility on the Disp-Char strings, or we need
> to put that info somewhere else.

I hope only handling the new flag part is ugly. It can be hidden
in the PQconninfoMapping[] array and PQconninfo(conn, true)
pre-filters the list as in the attached version.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

Attachment

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Amit Kapila

Date:

10 October 2012, 10:00:37

On Thursday, October 04, 2012 9:50 PM Boszormenyi Zoltan
> 2012-10-04 16:43 keltezéssel, Tom Lane írta:
> > Boszormenyi Zoltan <zb@cybertec.at> writes:
> >>> Did you think about something like the attached code?
> >> Or rather this one, which fixes a bug so fillPGconn() and
> >> PQconninfo() are symmetric and work for "requiressl".
> > That's incredibly ugly.  I'm not sure where we should keep the "R"
> > information, but shoehorning it into the existing PQconninfoOption
> > struct like that seems totally unacceptable.  Either we're willing to
> > break backwards compatibility on the Disp-Char strings, or we need to
> > put that info somewhere else.
>
> I hope only handling the new flag part is ugly. It can be hidden in the
> PQconninfoMapping[] array and PQconninfo(conn, true) pre-filters the
> list as in the attached version.

Please find the review of the patch.

Basic stuff:
------------
- patch apply failed at exports.txt file. Needs rebase to the current
master.
- Compiles cleanly with no warnings


What it does:
--------------
pg_basebackup does the base backup from the primary machine and also writes
the recovery.conf file with the option -R,
which is used for the standby to connect to primary for streaming
replication.

Testing:
---------
1. Test pg_basebackup with option -R to check that the recovery.conf file is
written to data directory.    --recovery.conf file is created in data directory.
2. Test pg_basebackup with option -R to check that the recovery.conf file is
not able to create because of disk full.    --Error is given as recovery.conf file is not able to create.
3. Test pg_basebackup with option -R including -h, -U, -p, -w and -W.   verify the recovery.conf which is created in
datadirectory.    --All the primary connection info parameters are working fine.     
4. Test pg_basebackup with conflict options (-x or -X and -R).    --Error is given when the conflict options are
providedto 
pg_basebackup.

5. Test pg_basebackup with option -R from a standby server.    --recovery.conf file is created in data directory.

Code Review:
-------------
1.
typedef struct PQconninfoMapping {
+        char                *keyword;
+        size_t                member_offset;
+        bool                for_replication;
+        char                *conninfoValue;        /* Special value mapping
*/
+        char                *connValue;
+} PQconninfoMapping;

Provide the better explanation of conninfoValue and connValue, how and where
these are used?

2. if (tmp && strncmp(tmp, mapping->conninfoValue, len) == 0)

In any case if the above condition is not satisfied then the PGconn data is
not filled with PQconninfoOption.
Is it correct?


Documentation:
-------------
1.
+        <para>
+       The <literal>for_replication</> argument can be used to exclude some

+       options from the list which are used by the walreceiver module.
+       <application>pg_basebackup</application> uses this pre-filtered list

+       to construct <literal>primary_conninfo</> in the automatically
generated
+       recovery.conf file.
+      </para>

I feel the explanation should be as follows,
exclude some options from the list which are not used by the walreceiver
module?


Observations/Issues:
-------------------
1. If the password contains any escape sequence characters, It is leading to
problems while walreceiver connecting to primary   by using the primary conninfo from recovery.conf
  please log an warning message or a note in document to handle such a case
manually by the user.


With Regards,
Amit Kapila.

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

10 October 2012, 16:14:03

Hi,

thanks for the new review.

2012-10-10 08:58 keltezéssel, Amit Kapila írta:
> On Thursday, October 04, 2012 9:50 PM Boszormenyi Zoltan
>> 2012-10-04 16:43 keltezéssel, Tom Lane írta:
>>> Boszormenyi Zoltan <zb@cybertec.at> writes:
>>>>> Did you think about something like the attached code?
>>>> Or rather this one, which fixes a bug so fillPGconn() and
>>>> PQconninfo() are symmetric and work for "requiressl".
>>> That's incredibly ugly.  I'm not sure where we should keep the "R"
>>> information, but shoehorning it into the existing PQconninfoOption
>>> struct like that seems totally unacceptable.  Either we're willing to
>>> break backwards compatibility on the Disp-Char strings, or we need to
>>> put that info somewhere else.
>> I hope only handling the new flag part is ugly. It can be hidden in the
>> PQconninfoMapping[] array and PQconninfo(conn, true) pre-filters the
>> list as in the attached version.
> Please find the review of the patch.
>
> Basic stuff:
> ------------
> - patch apply failed at exports.txt file. Needs rebase to the current
> master.

Done.

> - Compiles cleanly with no warnings
>
>
> What it does:
> --------------
> pg_basebackup does the base backup from the primary machine and also writes
> the recovery.conf file with the option -R,
> which is used for the standby to connect to primary for streaming
> replication.
>
> Testing:
> ---------
> 1. Test pg_basebackup with option -R to check that the recovery.conf file is
> written to data directory.
>      --recovery.conf file is created in data directory.
>
> 2. Test pg_basebackup with option -R to check that the recovery.conf file is
> not able to create because of disk full.
>      --Error is given as recovery.conf file is not able to create.
>
> 3. Test pg_basebackup with option -R including -h, -U, -p, -w and -W.
>     verify the recovery.conf which is created in data directory.
>      --All the primary connection info parameters are working fine.
>
> 4. Test pg_basebackup with conflict options (-x or -X and -R).
>      --Error is given when the conflict options are provided to
> pg_basebackup.
>
> 5. Test pg_basebackup with option -R from a standby server.
>      --recovery.conf file is created in data directory.
>
>
> Code Review:
> -------------
> 1.
> typedef struct PQconninfoMapping {
> +        char                *keyword;
> +        size_t                member_offset;
> +        bool                for_replication;
> +        char                *conninfoValue;        /* Special value mapping
> */
> +        char                *connValue;
> +} PQconninfoMapping;
>
> Provide the better explanation of conninfoValue and connValue, how and where
> these are used?

OK. This is only used for " requiressl='1' " (in the connection string)
and if '1' is set, PGconn.sslmode will be set to "require". All other
values are treated as it's being unset. This simplistic mapping
is used because there is only one such setting where different values
are used on the conninfo and the PGconn sides.

> 2. if (tmp && strncmp(tmp, mapping->conninfoValue, len) == 0)
>
> In any case if the above condition is not satisfied then the PGconn data is
> not filled with PQconninfoOption.
> Is it correct?

Yes, it stays NULL as makeEmptyPGconn() initialized it. This case only happens
with the "requiressl" setting with its special mapping. If you set " requiressl = '0' "
then it means that " requiressl='1' " was not set so the PGconn side stays as NULL.

The special casing was there in the old code too and behaves the same.

> Documentation:
> -------------
> 1.
> +        <para>
> +       The <literal>for_replication</> argument can be used to exclude some
>
> +       options from the list which are used by the walreceiver module.
> +       <application>pg_basebackup</application> uses this pre-filtered list
>
> +       to construct <literal>primary_conninfo</> in the automatically
> generated
> +       recovery.conf file.
> +      </para>
>
> I feel the explanation should be as follows,
> exclude some options from the list which are not used by the walreceiver
> module?

Err, no. The list excludes those[1] that *are* used (would be
overridden) by the walreceiver module:

----8<--------8<--------8<--------8<--------8<----
static bool
libpqrcv_connect(char *conninfo, XLogRecPtr startpoint)
{
...
         snprintf(conninfo_repl, sizeof(conninfo_repl),
                          "%s dbname=replication replication=true
fallback_application_name=walreceiver",
                          conninfo);
----8<--------8<--------8<--------8<--------8<----

[1] Actually, more than these 3 options are excluded. The deprecated
ones are also excluded.

> Observations/Issues:
> -------------------
> 1. If the password contains any escape sequence characters, It is leading to
> problems while walreceiver connecting to primary
>     by using the primary conninfo from recovery.conf
>
>     please log an warning message or a note in document to handle such a case
> manually by the user.

Done at both places.

Also, to adapt to the style of other error messages, now all my fprintf() statements
are prefixed with: "%s: ...", progname.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

2012-10-10 20:36 keltezéssel, Boszormenyi Zoltan írta:
> 2012-10-10 18:23 keltezéssel, Fujii Masao írta:
>> On Wed, Oct 10, 2012 at 10:12 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>>> Hi,
>>>
>>> thanks for the new review.
>>>
>>> 2012-10-10 08:58 keltezéssel, Amit Kapila írta:
>>>> On Thursday, October 04, 2012 9:50 PM Boszormenyi Zoltan
>>>>> 2012-10-04 16:43 keltezéssel, Tom Lane írta:
>>>>>
>>>>>> Boszormenyi Zoltan <zb@cybertec.at> writes:
>>>>>>>> Did you think about something like the attached code?
>>>>>>> Or rather this one, which fixes a bug so fillPGconn() and
>>>>>>> PQconninfo() are symmetric and work for "requiressl".
>>>>>> That's incredibly ugly.  I'm not sure where we should keep the "R"
>>>>>> information, but shoehorning it into the existing PQconninfoOption
>>>>>> struct like that seems totally unacceptable.  Either we're willing to
>>>>>> break backwards compatibility on the Disp-Char strings, or we need to
>>>>>> put that info somewhere else.
>>>>> I hope only handling the new flag part is ugly. It can be hidden in the
>>>>> PQconninfoMapping[] array and PQconninfo(conn, true) pre-filters the
>>>>> list as in the attached version.
>>>> Please find the review of the patch.
>>>>
>>>>
>>>> Basic stuff:
>>>> ------------
>>>> - patch apply failed at exports.txt file. Needs rebase to the current
>>>> master.
>>>
>>> Done.
>>>
>>>> - Compiles cleanly with no warnings
>>>>
>>>>
>>>> What it does:
>>>> --------------
>>>> pg_basebackup does the base backup from the primary machine and also
>>>> writes
>>>> the recovery.conf file with the option -R,
>>>> which is used for the standby to connect to primary for streaming
>>>> replication.
>>>>
>>>> Testing:
>>>> ---------
>>>> 1. Test pg_basebackup with option -R to check that the recovery.conf file
>>>> is
>>>> written to data directory.
>>>>       --recovery.conf file is created in data directory.
>>>>       2. Test pg_basebackup with option -R to check that the recovery.conf
>>>> file is
>>>> not able to create because of disk full.
>>>>       --Error is given as recovery.conf file is not able to create.
>>>>       3. Test pg_basebackup with option -R including -h, -U, -p, -w and -W.
>>>>
>>>>      verify the recovery.conf which is created in data directory.
>>>>       --All the primary connection info parameters are working fine.
>>>>       4. Test pg_basebackup with conflict options (-x or -X and -R).
>>>>
>>>>       --Error is given when the conflict options are provided to
>>>> pg_basebackup.
>>>>
>>>> 5. Test pg_basebackup with option -R from a standby server.
>>>>       --recovery.conf file is created in data directory.
>>>>
>>>>           Code Review:
>>>> -------------
>>>> 1.
>>>> typedef struct PQconninfoMapping {
>>>> +        char                *keyword;
>>>> +        size_t                member_offset;
>>>> +        bool                for_replication;
>>>> +        char                *conninfoValue;        /* Special value
>>>> mapping
>>>> */
>>>> +        char                *connValue;
>>>> +} PQconninfoMapping;
>>>>
>>>> Provide the better explanation of conninfoValue and connValue, how and
>>>> where
>>>> these are used?
>>>
>>> OK. This is only used for " requiressl='1' " (in the connection string)
>>> and if '1' is set, PGconn.sslmode will be set to "require". All other
>>> values are treated as it's being unset. This simplistic mapping
>>> is used because there is only one such setting where different values
>>> are used on the conninfo and the PGconn sides.
>>>
>>>
>>>> 2. if (tmp && strncmp(tmp, mapping->conninfoValue, len) == 0)
>>>>
>>>> In any case if the above condition is not satisfied then the PGconn data
>>>> is
>>>> not filled with PQconninfoOption.
>>>> Is it correct?
>>>
>>> Yes, it stays NULL as makeEmptyPGconn() initialized it. This case only
>>> happens
>>> with the "requiressl" setting with its special mapping. If you set "
>>> requiressl = '0' "
>>> then it means that " requiressl='1' " was not set so the PGconn side stays
>>> as NULL.
>>>
>>> The special casing was there in the old code too and behaves the same.
>>>
>>>
>>>> Documentation:
>>>> -------------
>>>> 1.
>>>> +        <para>
>>>> +       The <literal>for_replication</> argument can be used to exclude
>>>> some
>>>>
>>>> +       options from the list which are used by the walreceiver module.
>>>> +       <application>pg_basebackup</application> uses this pre-filtered
>>>> list
>>>>
>>>> +       to construct <literal>primary_conninfo</> in the automatically
>>>> generated
>>>> +       recovery.conf file.
>>>> +      </para>
>>>>
>>>> I feel the explanation should be as follows,
>>>> exclude some options from the list which are not used by the walreceiver
>>>> module?
>>>
>>> Err, no. The list excludes those[1] that *are* used (would be
>>> overridden) by the walreceiver module:
>>>
>>> ----8<--------8<--------8<--------8<--------8<----
>>> static bool
>>> libpqrcv_connect(char *conninfo, XLogRecPtr startpoint)
>>> {
>>> ...
>>>          snprintf(conninfo_repl, sizeof(conninfo_repl),
>>>                           "%s dbname=replication replication=true
>>> fallback_application_name=walreceiver",
>>>                           conninfo);
>>> ----8<--------8<--------8<--------8<--------8<----
>>>
>>> [1] Actually, more than these 3 options are excluded. The deprecated
>>> ones are also excluded.
>>>
>>>
>>>> Observations/Issues:
>>>> -------------------
>>>> 1. If the password contains any escape sequence characters, It is leading
>>>> to
>>>> problems while walreceiver connecting to primary
>>>>      by using the primary conninfo from recovery.conf
>>>>
>>>>      please log an warning message or a note in document to handle such a
>>>> case
>>>> manually by the user.
>>>
>>> Done at both places.
>>>
>>> Also, to adapt to the style of other error messages, now all my fprintf()
>>> statements
>>> are prefixed with: "%s: ...", progname.
>> In new patches, when I ran "pg_basebackup -D hoge -c fast -R" on MacOS,
>> I got the following error message. BTW, I compiled the patched PostgreSQL
>> with --enable-debug and --enable-cassert options.
>>
>> pg_basebackup(41751) malloc: *** error for object 0x106001af0: pointer
>> being freed was not allocated
>> *** set a breakpoint in malloc_error_break to debug
>> Abort trap: 6
>
> Can you show a backtrace? I compiled it on Fedora 17/x86_64 with
> --enable-depend --enable-debug --enable-cassert. GLIBC is also smart
> enough to catch improper free() calls, too.

I was able to test it on OSX and I found my bug. Attached is the new code.
The problem was in conninfo_init(), the last entry in the filtered list was
not initialized to 0. It seems that for some reason, my Linux machine gave
me pre-initialized memory.

>
>> $ uname -a
>> Darwin hrk.local 11.4.2 Darwin Kernel Version 11.4.2: Thu Aug 23
>> 16:25:48 PDT 2012; root:xnu-1699.32.7~1/RELEASE_X86_64 x86_64
>>
>>
>> When tar output format is specified together with -R option, recovery.conf is
>> not included in base.tar. I think it should.
>
> Why? This patch only promises to write the recovery.conf into the
> directory specified with -D.
>
>> +        setting up the standby. Since creating a backup for a standalone
>> +        server with <option>-x</option> or <option>-X</option> and adding
>> +        a recovery.conf to it wouldn't make a working standby, these options
>> +        naturally conflict.
>>
>> Is this right? ISTM that basically we can use pg_basebackup -x to take
>> a base backup for starting the standby for now. No?
>
> I don't know. Pointers?
>
> Thanks,
> Zoltán Böszörményi
>


--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

2012-10-12 22:21 keltezéssel, Robert Haas írta:
> On Wed, Oct 10, 2012 at 8:02 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Thu, Oct 11, 2012 at 3:36 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>>> 2012-10-10 18:23 keltezéssel, Fujii Masao írta:
>>>> When tar output format is specified together with -R option, recovery.conf
>>>> is
>>>> not included in base.tar. I think it should.
>>>
>>> Why? This patch only promises to write the recovery.conf into the
>>> directory specified with -D.
>> Because it's more user-friendly. If recovery.conf is not included in base.tar,
>> when base.tar is extracted to disk to use the backup, a user always needs
>> to copy recovery.conf to the extracted directory. OTOH if it's included in
>> base.tar, such copy operation is not required and we can simplify the
>> procedures to use the backup a bit.
> +1.

OK, out of popular demand, I implemented writing into the base.tar
if both -R and -Ft was specified.

The code to create a tar header and the checksum inside the header
was copied from bin/pg_dump/pg_backup_tar.c.

I tested these scenarios ("server" can be either a master and standby):
- backup a server with -Fp
    (no recovery.conf in the target directory was written)
- backup a server with -Ftar
    (no recovery.conf was written into the target directory or base.tar)
- backup a server with -Ftar -z
    (no recovery.conf was written into the target directory or base.tar.gz)
- backup a server with -R -Fp
    (the new recovery.conf was written into the target directory)
- backup a server with -R -Ftar
    (the new recovery.conf was written into the base.tar)
- backup a server with -R -Ftar -z
    (the new recovery.conf was written into the base.tar.gz)

Backing up a standby server without -R preserves the original recovery.conf of the
standby, it points to the standby's source server.

Backing up a standby server with -R overwrites the original recovery.conf with the new
one pointing to the standby instead of the standby's source server. Without -Ft, it is
obvious. With -Ft, there are two recovery.conf files in the tar file and upon extracting it,
the last written one (the one generated via -R) overwrites the original.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

Hi,

2012-10-14 18:41 keltezéssel, Boszormenyi Zoltan írta:
> 2012-10-14 18:02 keltezéssel, Fujii Masao írta:
>> Thanks for updating the patch!
>>
>> On Sun, Oct 14, 2012 at 8:41 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>>> Backing up a standby server without -R preserves the original recovery.conf
>>> of the
>>> standby, it points to the standby's source server.
>>>
>>> Backing up a standby server with -R overwrites the original recovery.conf
>>> with the new
>>> one pointing to the standby instead of the standby's source server. Without
>>> -Ft, it is
>>> obvious. With -Ft, there are two recovery.conf files in the tar file and
>>> upon extracting it,
>>> the last written one (the one generated via -R) overwrites the original.
>> The tar file is always extracted such way in all platform which PostgreSQL
>> supports? I'm just concerned about that some tool in some platform might
>> prefer the original recovery.conf when extracting tar file. If the spec of tar
>> format specifies such behavior (i.e., the last written file of the same name
>> is always preferred), it's OK.
>
> Since tar is a sequential archive format, I think this is the behaviour of
> every tar extractor. But I will look at adding code to skip the original
> recovery.conf if it exists in the tar file.
>
>> I found the bug that recovery.conf is included in the tar file of the tablespace
>> instead of base.tar, when there are tablespaces in the server.
>
> You are right, I am looking into this. But I don't know how it got there,
> I check for (rownum == 0 && writerecoveryconf) and rownum == 0
> supposedly means that it's the base.tar. Looking again.

I made a mistake in the previous check, rownum is not reliable.
The tablespaces are sent first and base backup as the last.
Now recovery.conf is written into base.tar.

>> Maybe this is nitpicky problem,,,, but...
>> If port number is not explicitly specified in pg_basebackup, the port
>> number is not
>> included to primary_conninfo in recovery.conf which is created during
>> the backup.
>> That is, the standby server using such recovery.conf tries to connect
>> to the default
>> port number because the port number is not supplied in primary_conninfo. This
>> assumes that the default port number is the same between the master and standby.
>> But this is not true. The default port number can be changed in --with-pgport
>> configure option, so the default port number might be different
>> between the master
>> and standby. To avoid this uncertainty, pg_basebackup -R should always include
>> the port number in primary_conninfo?
>
> I think you are right. But, I wouldn't restrict it only to the port setting.
> Any of the values that are set and equal to the compiled-in default,
> it should be written into recovery.conf.

Now all values that are set (even those being equal to the compiled-in default)
are put into recovery.conf.

>> When the password is required to connect to the server, pg_basebackup -R
>> always writes the password setting into primary_conninfo in recovery.conf.
>> But if the password is supplied from .pgpass, ISTM that the password setting
>> doesn't need to be written into primary_conninfo. Right?
>
> How can you deduce it from the PQconninfoOption structure?
>
> Also, if the machine you take the base backup on is different
> from the one where you actually use the backup on, it can be
> different not only in the --with-pgport compilation option but
> in the presence of .pgpass or the PGPASSWORD envvar, too.
> The administrator is there for a reason or there is no .pgpass
> or PGPASSWORD at all.
>
>> +        The password written into recovery.conf is not escaped even if special
>> +        characters appear in it. The administrator must review recovery.conf
>> +        to ensure proper escaping.
>>
>> Is it difficult to make pg_basebackup escape the special characters in the
>> password? It's better if we can remove this restriction.
>
> It's not difficult. What other characters need to be escaped besides single quotes?

All written values are escaped.

Other changes: the recovery.conf in base.tar is correctly skipped if it exists
and -R is given. The new recovery.conf is written with padding to round up to
512, the TAR chunk size.

The PQconninfo patch is also attached but didn't change since the last mail.

>
>> I've not reviewed PQconninfo patch yet. Will review.
>
> Thanks in advance.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

Attachment

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

14 October 2012, 23:26:27

2012-10-14 22:23 keltezéssel, Boszormenyi Zoltan írta:
> Hi,
>
> 2012-10-14 18:41 keltezéssel, Boszormenyi Zoltan írta:
>> 2012-10-14 18:02 keltezéssel, Fujii Masao írta:
>>> Thanks for updating the patch!
>>>
>>> On Sun, Oct 14, 2012 at 8:41 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>>>> Backing up a standby server without -R preserves the original recovery.conf
>>>> of the
>>>> standby, it points to the standby's source server.
>>>>
>>>> Backing up a standby server with -R overwrites the original recovery.conf
>>>> with the new
>>>> one pointing to the standby instead of the standby's source server. Without
>>>> -Ft, it is
>>>> obvious. With -Ft, there are two recovery.conf files in the tar file and
>>>> upon extracting it,
>>>> the last written one (the one generated via -R) overwrites the original.
>>> The tar file is always extracted such way in all platform which PostgreSQL
>>> supports? I'm just concerned about that some tool in some platform might
>>> prefer the original recovery.conf when extracting tar file. If the spec of tar
>>> format specifies such behavior (i.e., the last written file of the same name
>>> is always preferred), it's OK.
>>
>> Since tar is a sequential archive format, I think this is the behaviour of
>> every tar extractor. But I will look at adding code to skip the original
>> recovery.conf if it exists in the tar file.
>>
>>> I found the bug that recovery.conf is included in the tar file of the tablespace
>>> instead of base.tar, when there are tablespaces in the server.
>>
>> You are right, I am looking into this. But I don't know how it got there,
>> I check for (rownum == 0 && writerecoveryconf) and rownum == 0
>> supposedly means that it's the base.tar. Looking again.
>
> I made a mistake in the previous check, rownum is not reliable.
> The tablespaces are sent first and base backup as the last.
> Now recovery.conf is written into base.tar.
>
>>> Maybe this is nitpicky problem,,,, but...
>>> If port number is not explicitly specified in pg_basebackup, the port
>>> number is not
>>> included to primary_conninfo in recovery.conf which is created during
>>> the backup.
>>> That is, the standby server using such recovery.conf tries to connect
>>> to the default
>>> port number because the port number is not supplied in primary_conninfo. This
>>> assumes that the default port number is the same between the master and standby.
>>> But this is not true. The default port number can be changed in --with-pgport
>>> configure option, so the default port number might be different
>>> between the master
>>> and standby. To avoid this uncertainty, pg_basebackup -R should always include
>>> the port number in primary_conninfo?
>>
>> I think you are right. But, I wouldn't restrict it only to the port setting.
>> Any of the values that are set and equal to the compiled-in default,
>> it should be written into recovery.conf.
>
> Now all values that are set (even those being equal to the compiled-in default)
> are put into recovery.conf.
>
>>> When the password is required to connect to the server, pg_basebackup -R
>>> always writes the password setting into primary_conninfo in recovery.conf.
>>> But if the password is supplied from .pgpass, ISTM that the password setting
>>> doesn't need to be written into primary_conninfo. Right?
>>
>> How can you deduce it from the PQconninfoOption structure?
>>
>> Also, if the machine you take the base backup on is different
>> from the one where you actually use the backup on, it can be
>> different not only in the --with-pgport compilation option but
>> in the presence of .pgpass or the PGPASSWORD envvar, too.
>> The administrator is there for a reason or there is no .pgpass
>> or PGPASSWORD at all.
>>
>>> +        The password written into recovery.conf is not escaped even if special
>>> +        characters appear in it. The administrator must review recovery.conf
>>> +        to ensure proper escaping.
>>>
>>> Is it difficult to make pg_basebackup escape the special characters in the
>>> password? It's better if we can remove this restriction.
>>
>> It's not difficult. What other characters need to be escaped besides single quotes?
>
> All written values are escaped.
>
> Other changes: the recovery.conf in base.tar is correctly skipped if it exists
> and -R is given. The new recovery.conf is written with padding to round up to
> 512, the TAR chunk size.

Also, the check for conflict between -R and -x/-X is now removed.

>
> The PQconninfo patch is also attached but didn't change since the last mail.
>
>>
>>> I've not reviewed PQconninfo patch yet. Will review.
>>
>> Thanks in advance.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

14 October 2012, 23:31:54

2012-10-14 22:26 keltezéssel, Boszormenyi Zoltan írta:
> 2012-10-14 22:23 keltezéssel, Boszormenyi Zoltan írta:
>> Hi,
>>
>> 2012-10-14 18:41 keltezéssel, Boszormenyi Zoltan írta:
>>> 2012-10-14 18:02 keltezéssel, Fujii Masao írta:
>>>> Thanks for updating the patch!
>>>>
>>>> On Sun, Oct 14, 2012 at 8:41 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>>>>> Backing up a standby server without -R preserves the original recovery.conf
>>>>> of the
>>>>> standby, it points to the standby's source server.
>>>>>
>>>>> Backing up a standby server with -R overwrites the original recovery.conf
>>>>> with the new
>>>>> one pointing to the standby instead of the standby's source server. Without
>>>>> -Ft, it is
>>>>> obvious. With -Ft, there are two recovery.conf files in the tar file and
>>>>> upon extracting it,
>>>>> the last written one (the one generated via -R) overwrites the original.
>>>> The tar file is always extracted such way in all platform which PostgreSQL
>>>> supports? I'm just concerned about that some tool in some platform might
>>>> prefer the original recovery.conf when extracting tar file. If the spec of tar
>>>> format specifies such behavior (i.e., the last written file of the same name
>>>> is always preferred), it's OK.
>>>
>>> Since tar is a sequential archive format, I think this is the behaviour of
>>> every tar extractor. But I will look at adding code to skip the original
>>> recovery.conf if it exists in the tar file.
>>>
>>>> I found the bug that recovery.conf is included in the tar file of the tablespace
>>>> instead of base.tar, when there are tablespaces in the server.
>>>
>>> You are right, I am looking into this. But I don't know how it got there,
>>> I check for (rownum == 0 && writerecoveryconf) and rownum == 0
>>> supposedly means that it's the base.tar. Looking again.
>>
>> I made a mistake in the previous check, rownum is not reliable.
>> The tablespaces are sent first and base backup as the last.
>> Now recovery.conf is written into base.tar.
>>
>>>> Maybe this is nitpicky problem,,,, but...
>>>> If port number is not explicitly specified in pg_basebackup, the port
>>>> number is not
>>>> included to primary_conninfo in recovery.conf which is created during
>>>> the backup.
>>>> That is, the standby server using such recovery.conf tries to connect
>>>> to the default
>>>> port number because the port number is not supplied in primary_conninfo. This
>>>> assumes that the default port number is the same between the master and standby.
>>>> But this is not true. The default port number can be changed in --with-pgport
>>>> configure option, so the default port number might be different
>>>> between the master
>>>> and standby. To avoid this uncertainty, pg_basebackup -R should always include
>>>> the port number in primary_conninfo?
>>>
>>> I think you are right. But, I wouldn't restrict it only to the port setting.
>>> Any of the values that are set and equal to the compiled-in default,
>>> it should be written into recovery.conf.
>>
>> Now all values that are set (even those being equal to the compiled-in default)
>> are put into recovery.conf.
>>
>>>> When the password is required to connect to the server, pg_basebackup -R
>>>> always writes the password setting into primary_conninfo in recovery.conf.
>>>> But if the password is supplied from .pgpass, ISTM that the password setting
>>>> doesn't need to be written into primary_conninfo. Right?
>>>
>>> How can you deduce it from the PQconninfoOption structure?
>>>
>>> Also, if the machine you take the base backup on is different
>>> from the one where you actually use the backup on, it can be
>>> different not only in the --with-pgport compilation option but
>>> in the presence of .pgpass or the PGPASSWORD envvar, too.
>>> The administrator is there for a reason or there is no .pgpass
>>> or PGPASSWORD at all.
>>>
>>>> +        The password written into recovery.conf is not escaped even if special
>>>> +        characters appear in it. The administrator must review recovery.conf
>>>> +        to ensure proper escaping.
>>>>
>>>> Is it difficult to make pg_basebackup escape the special characters in the
>>>> password? It's better if we can remove this restriction.
>>>
>>> It's not difficult. What other characters need to be escaped besides single quotes?
>>
>> All written values are escaped.
>>
>> Other changes: the recovery.conf in base.tar is correctly skipped if it exists
>> and -R is given. The new recovery.conf is written with padding to round up to
>> 512, the TAR chunk size.
>
> Also, the check for conflict between -R and -x/-X is now removed.

Really the last one, for today at least. The buffer for recovery.conf is freed
in both the -Fp and -Ft cases.

>
>>
>> The PQconninfo patch is also attached but didn't change since the last mail.
>>
>>>
>>>> I've not reviewed PQconninfo patch yet. Will review.
>>>
>>> Thanks in advance.
>
> Best regards,
> Zoltán Böszörményi
>


--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

Attachment

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

15 October 2012, 10:47:14

2012-10-14 22:31 keltezéssel, Boszormenyi Zoltan írta:
> 2012-10-14 22:26 keltezéssel, Boszormenyi Zoltan írta:
>> 2012-10-14 22:23 keltezéssel, Boszormenyi Zoltan írta:
>>> Hi,
>>>
>>> 2012-10-14 18:41 keltezéssel, Boszormenyi Zoltan írta:
>>>> 2012-10-14 18:02 keltezéssel, Fujii Masao írta:
>>>>> Thanks for updating the patch!
>>>>>
>>>>> On Sun, Oct 14, 2012 at 8:41 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>>>>>> Backing up a standby server without -R preserves the original recovery.conf
>>>>>> of the
>>>>>> standby, it points to the standby's source server.
>>>>>>
>>>>>> Backing up a standby server with -R overwrites the original recovery.conf
>>>>>> with the new
>>>>>> one pointing to the standby instead of the standby's source server. Without
>>>>>> -Ft, it is
>>>>>> obvious. With -Ft, there are two recovery.conf files in the tar file and
>>>>>> upon extracting it,
>>>>>> the last written one (the one generated via -R) overwrites the original.
>>>>> The tar file is always extracted such way in all platform which PostgreSQL
>>>>> supports? I'm just concerned about that some tool in some platform might
>>>>> prefer the original recovery.conf when extracting tar file. If the spec of tar
>>>>> format specifies such behavior (i.e., the last written file of the same name
>>>>> is always preferred), it's OK.
>>>>
>>>> Since tar is a sequential archive format, I think this is the behaviour of
>>>> every tar extractor. But I will look at adding code to skip the original
>>>> recovery.conf if it exists in the tar file.
>>>>
>>>>> I found the bug that recovery.conf is included in the tar file of the tablespace
>>>>> instead of base.tar, when there are tablespaces in the server.
>>>>
>>>> You are right, I am looking into this. But I don't know how it got there,
>>>> I check for (rownum == 0 && writerecoveryconf) and rownum == 0
>>>> supposedly means that it's the base.tar. Looking again.
>>>
>>> I made a mistake in the previous check, rownum is not reliable.
>>> The tablespaces are sent first and base backup as the last.
>>> Now recovery.conf is written into base.tar.
>>>
>>>>> Maybe this is nitpicky problem,,,, but...
>>>>> If port number is not explicitly specified in pg_basebackup, the port
>>>>> number is not
>>>>> included to primary_conninfo in recovery.conf which is created during
>>>>> the backup.
>>>>> That is, the standby server using such recovery.conf tries to connect
>>>>> to the default
>>>>> port number because the port number is not supplied in primary_conninfo. This
>>>>> assumes that the default port number is the same between the master and standby.
>>>>> But this is not true. The default port number can be changed in --with-pgport
>>>>> configure option, so the default port number might be different
>>>>> between the master
>>>>> and standby. To avoid this uncertainty, pg_basebackup -R should always include
>>>>> the port number in primary_conninfo?
>>>>
>>>> I think you are right. But, I wouldn't restrict it only to the port setting.
>>>> Any of the values that are set and equal to the compiled-in default,
>>>> it should be written into recovery.conf.
>>>
>>> Now all values that are set (even those being equal to the compiled-in default)
>>> are put into recovery.conf.
>>>
>>>>> When the password is required to connect to the server, pg_basebackup -R
>>>>> always writes the password setting into primary_conninfo in recovery.conf.
>>>>> But if the password is supplied from .pgpass, ISTM that the password setting
>>>>> doesn't need to be written into primary_conninfo. Right?
>>>>
>>>> How can you deduce it from the PQconninfoOption structure?
>>>>
>>>> Also, if the machine you take the base backup on is different
>>>> from the one where you actually use the backup on, it can be
>>>> different not only in the --with-pgport compilation option but
>>>> in the presence of .pgpass or the PGPASSWORD envvar, too.
>>>> The administrator is there for a reason or there is no .pgpass
>>>> or PGPASSWORD at all.
>>>>
>>>>> +        The password written into recovery.conf is not escaped even if special
>>>>> +        characters appear in it. The administrator must review recovery.conf
>>>>> +        to ensure proper escaping.
>>>>>
>>>>> Is it difficult to make pg_basebackup escape the special characters in the
>>>>> password? It's better if we can remove this restriction.
>>>>
>>>> It's not difficult. What other characters need to be escaped besides single quotes?
>>>
>>> All written values are escaped.
>>>
>>> Other changes: the recovery.conf in base.tar is correctly skipped if it exists
>>> and -R is given. The new recovery.conf is written with padding to round up to
>>> 512, the TAR chunk size.
>>
>> Also, the check for conflict between -R and -x/-X is now removed.

The documentation for option -R has changed to reflect this and
there is no different error code 2 now: it would make the behaviour
inconsistent between -Fp and -Ft.

>>> The PQconninfo patch is also attached but didn't change since the last mail.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

2012-11-18 17:20 keltezéssel, Magnus Hagander írta:
> On Tue, Oct 23, 2012 at 5:08 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Oct 23, 2012 4:52 PM, "Alvaro Herrera" <alvherre@2ndquadrant.com> wrote:
>>> Boszormenyi Zoltan escribió:
>>>
>>>>>> Also, the check for conflict between -R and -x/-X is now removed.
>>>> The documentation for option -R has changed to reflect this and
>>>> there is no different error code 2 now: it would make the behaviour
>>>> inconsistent between -Fp and -Ft.
>>>>
>>>>>>> The PQconninfo patch is also attached but didn't change since the
>>>>>>> last mail.
>>> Magnus,
>>>
>>> This patch is all yours to handle.  I'm guessing nothing will happen
>>> until pgconf.eu is done and over, but hopefully you can share a few
>>> beers with Zoltan over the whole subject (and maybe with Peter about the
>>> PQconninfo stuff?)
>>>
>>> I'm not closing this just yet, but if you're not able to handle this
>>> soon, maybe it'd be better to punt it to the November commitfest.
>> It's on my to do list for when I get back, but correct, won't get to it
>> until after the conference.
> I finally got around to looking at this patch now. Sorry about the way
> too long delay.
>
> A few thoughts:
>
> First, on the libpq patch:
>
> I'm not sure I like the for_replication flag to PQconninfo(). It seems
> we're it's a quite limited API, and not very future proof. What's to
> say that an app would only be interested in filtering based on
> for_replication? One idea might be to have a bitmap field there, and
> assign *all* conninfo options to a category. We could then have
> categories for NORMAL and REPLICATION. But we could also for example
> have a category for PASSWORD (or similar), so that you could get with
> and without those. Passing in a 32-bit integer would allow us to have
> 32 different categories, and we could then use a bitmask to pick
> things out.
>
> It might sound a bit like overengineering, but it's also an API and
> it's a PITA to change it in the future if more needs show up..

Check.

> Second, I wonder if we really need to add the code for requiressl=1,
> or if we should just remove it. The spelling requiressl=1 was
> deprecated back in 7.4 - which has obviously been out of support for a
> long time now.

I removed this option, the code is simpler, thanks to this.

> Third, in fillPGconn. If mapping has both conninfoValue and connvalue,
> it does a free() on the old value in memberaddr, but if it doesn't it
> just overwrites memberaddr with strdup(). Shouldn't those two paths be
> the same, meaning shouldn't the  if (*memberaddr) free(*memberaddr);
> check be outside the if block?

This point is now moot, see above.

> Fourth, I'm not sure I like the "memberaddr" term. It's not wrong, but
> it threw me off a couple of times while reading it. It's not all that
> important, and I'm not sure about another idea for it though - maybe
> just "connmember"?

The variable is now "connmember".

Also, I noticed that there was already a conninfo_storeval(),
the new patch uses it and there's no need to introduce a
new conninfo_setval() function.

> Then, about the pg_basebackup patch:
>
> What's the reason for the () around 512 for TARCHUNKSZ?

Removed the () from around the value.

> We have a lot of hardcoded entries of the 512 for tar in that file. We
> should either keep using 512 as a constant, or change all those to
> *also* use the #define. Right now, the code will obviously break if
> you change the #define (for example, it compares to 512, but then uses
> hdrleft = TARCHUNKSZ - tarhdrsz; to do calculation).

All 512 constants are now using the #define.

> The name choice of "basebackup" for the bool in ReceiveTarFile() is
> unfortunate, since we use the term base backup to refer to the
> complete thing, not just the main tablespace. Probably
> "basetablespcae" instead. And it should then be assigned at the top of
> the function (to the result of PQgetisnull()), and used in the main
> if() branch as well.

Done without your typo, so the variable is "basetablespace". ;-)

> Should we really print the status message even if not in verbose mode?
> We only print the "base backup complete" messages in verbose mode, for
> example.

The message is written only in verbose mode now.

> It seems wrong to free() recoveryconf in ReceiveTarFile(). It's
> allocated globally at the beginning. While that loop should only be
> called once (since only one tablespace can be the main one), it's a
> confusing location for the free.

See below.

> The whole tar writing part of the code needs a lot more comments. It's
> entirely unclear what the code does there. Why does the recovery.conf
> writing code need to be split up in multiple locations inside
> ReceiveTarFile(), for example? It either needs to be simplified, or
> explained why it can't be simplified in comments.
>
> _tarCreateHeader() is really badly named, since it specifically
> creates a tar header for recovery.conf only. Either that needs to be a
> parameter, or it needs to have a name that indicates this is the only
> thing it does. The former is probably better.

_tarCreateHeader() now accepts the file name and the file size arguments.

> Much of the tar stuff is very similar (I haven't looked to see if it's
> identical) to the stuff in backend/replication/basebackup.c. Perhaps
> we should have a src/port/tarutil.c?

I will implement it as a separate patch.

> escape_string() - already exists as escape_quotes() in initdb, AFAICT.
> We should either move it to src/port/, or at least copy it so it's
> exactly the same.

A copy of escape_quotes() is now in pg_basebackup.c and is used.

I will also unify the copies of it in a separate patch.

> CreateRecoveryConf() should just use PQExpBuffer (in libpq), I think -
> that does away with a lot of code. We already use this from e.g.
> pg_dump, so there's a precedent for using internal code from libpq in
> frontends.

PQexpBuffer is used now and it's created and destroyed inside BaseBackup().

> Again, my apologies for this review taking so long. I will try to be
> more attentive to the next round :S

Please, review the new patches.

Best regards,
Zoltán Böszörményi

> --
>   Magnus Hagander
>   Me: http://www.hagander.net/
>   Work: http://www.redpill-linpro.com/
>
>


--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

Attachment

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

20 November 2012, 19:15:38

2012-11-20 17:03 keltezéssel, Boszormenyi Zoltan írta:
> 2012-11-18 17:20 keltezéssel, Magnus Hagander írta:
>
>> The whole tar writing part of the code needs a lot more comments. It's
>> entirely unclear what the code does there. Why does the recovery.conf
>> writing code need to be split up in multiple locations inside
>> ReceiveTarFile(), for example? It either needs to be simplified, or
>> explained why it can't be simplified in comments.

I also simplified (the multiple #ifdef blocks are moved out into a function
to make the code shorter) and added comments to this function.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

20 November 2012, 22:33:29

2012-11-20 17:03 keltezéssel, Boszormenyi Zoltan írta:
> 2012-11-18 17:20 keltezéssel, Magnus Hagander írta:
>
>> Much of the tar stuff is very similar (I haven't looked to see if it's
>> identical) to the stuff in backend/replication/basebackup.c. Perhaps
>> we should have a src/port/tarutil.c?
>
> I will implement it as a separate patch.

I implemented it, all 3 patches are attached now. Use this patchset,
the previously sent 1st patch had a bug, it called conninit_storeval()
with value == NULL arguments and it crashes on strdup(NULL).

>
>> escape_string() - already exists as escape_quotes() in initdb, AFAICT.
>> We should either move it to src/port/, or at least copy it so it's
>> exactly the same.
>
> A copy of escape_quotes() is now in pg_basebackup.c and is used.
>
> I will also unify the copies of it in a separate patch.

This one is not done yet, but a suggestion on which existing file
it can fit into or for a new src/port/filename is welcome.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

2012-11-21 15:29 keltezéssel, Tom Lane írta:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
>> pg_basebackup needs pg_malloc() to call disconnect_and_exit(1)
>> instead to quit cleanly and not leave an "unexpected EOF from client"
>> message in the server log. Which is a macro at the moment, but
>> has to be turned into a real function for the reasons below.
> man 2 atexit

It doesn't really help to make it easier. E.g: pg_strdup() in psql does this:

char *
pg_strdup(const char *string)
{
         char       *tmp;

         if (!string)
         {
                 psql_error("%s: pg_strdup: cannot duplicate null pointer (internal error)\n",
                                 pset.progname);
                 exit(EXIT_FAILURE);
         }
         tmp = strdup(string);
         if (!tmp)
         {
                 psql_error("out of memory\n");
                 exit(EXIT_FAILURE);
         }
         return tmp;
}

The function passed to atexit() still needs to know which string to print
and this needs an internal variable. Also, the actions taken on pg_malloc/pg_strdup
errors don't produce the exact same messages.

This is why I suggest the attached 03-pg_malloc-unified-v2.patch.

The 04-tarutils-v2.patch unifies the common tar functions and the string functions
like print_val() used by _tarCreateHeader (previously _tarWriteHeader) and
escape_quotes().

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

Attachment

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

22 November 2012, 13:07:33

Hi,

2012-11-21 18:39 keltezéssel, Boszormenyi Zoltan írta:
> 2012-11-21 15:29 keltezéssel, Tom Lane írta:
>> Boszormenyi Zoltan <zb@cybertec.at> writes:
>>> pg_basebackup needs pg_malloc() to call disconnect_and_exit(1)
>>> instead to quit cleanly and not leave an "unexpected EOF from client"
>>> message in the server log. Which is a macro at the moment, but
>>> has to be turned into a real function for the reasons below.
>> man 2 atexit
>
> It doesn't really help to make it easier. E.g: pg_strdup() in psql does this:
>
> char *
> pg_strdup(const char *string)
> {
>         char       *tmp;
>
>         if (!string)
>         {
>                 psql_error("%s: pg_strdup: cannot duplicate null pointer (internal
> error)\n",
>                                 pset.progname);
>                 exit(EXIT_FAILURE);
>         }
>         tmp = strdup(string);
>         if (!tmp)
>         {
>                 psql_error("out of memory\n");
>                 exit(EXIT_FAILURE);
>         }
>         return tmp;
> }
>
> The function passed to atexit() still needs to know which string to print
> and this needs an internal variable. Also, the actions taken on pg_malloc/pg_strdup
> errors don't produce the exact same messages.
>
> This is why I suggest the attached 03-pg_malloc-unified-v2.patch.
>
> The 04-tarutils-v2.patch unifies the common tar functions and the string functions
> like print_val() used by _tarCreateHeader (previously _tarWriteHeader) and
> escape_quotes().

The PQconninfo patch grew its own mail thread (this is why
there's no 1st patch in this series now) and I reworked the order
of the patches to make it more logical.  This made the pg_basebackup
patch 5K smaller.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

Attachment

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

22 November 2012, 19:57:44

2012-11-22 11:06 keltezéssel, Boszormenyi Zoltan írta:

Hi,

2012-11-21 18:39 keltezéssel, Boszormenyi Zoltan írta:
2012-11-21 15:29 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan <zb@cybertec.at> writes:
pg_basebackup needs pg_malloc() to call disconnect_and_exit(1)
instead to quit cleanly and not leave an "unexpected EOF from client"
message in the server log. Which is a macro at the moment, but
has to be turned into a real function for the reasons below.
man 2 atexit

It doesn't really help to make it easier. E.g: pg_strdup() in psql does this:

char *
pg_strdup(const char *string)
{
        char       *tmp;

        if (!string)
        {
                psql_error("%s: pg_strdup: cannot duplicate null pointer (internal error)\n",
                                pset.progname);
                exit(EXIT_FAILURE);
        }
        tmp = strdup(string);
        if (!tmp)
        {
                psql_error("out of memory\n");
                exit(EXIT_FAILURE);
        }
        return tmp;
}

The function passed to atexit() still needs to know which string to print
and this needs an internal variable. Also, the actions taken on pg_malloc/pg_strdup
errors don't produce the exact same messages.

This is why I suggest the attached 03-pg_malloc-unified-v2.patch.

The 04-tarutils-v2.patch unifies the common tar functions and the string functions
like print_val() used by _tarCreateHeader (previously _tarWriteHeader) and
escape_quotes().

The PQconninfo patch grew its own mail thread (this is why
there's no 1st patch in this series now) and I reworked the order
of the patches to make it more logical. This made the pg_basebackup
patch 5K smaller.

The changes here are:

02-pg_malloc-unified-v4.1.patch: fixed crashing PQfinish() at exit time
in pg_basebackup, added the atexit() initializer to pg_receivexlog

03-tarutils-strutils-v3.1.patch: rebased to avoid fuzz in patching

04-pg_basebackup-v18.1.patch: use Magnus' PQconninfo patch
and filter the PQconninfoOption array internally, also rebased

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de    http://www.postgresql.at/

Attachment

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

30 November 2012, 12:14:12

Hi,

now that PQconninfo() was committed, I rebased the subsequent
patches. Actual code change was only in the last patch, to
conform to the committed PQconninfo() API.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

Attachment

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Magnus Hagander

Date:

01 January 2013, 19:18:39

On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:

Hi,

now that PQconninfo() was committed, I rebased the subsequent
patches. Actual code change was only in the last patch, to
conform to the committed PQconninfo() API.

So, finally coming back to this one.

What happened to Alvaro's suggestion of:

> It seems simpler to have the escape_quotes utility function in port just
> not use pg_malloc at all, have it return NULL or similar failure
> indicator when malloc() fails, and then the caller decides what to do.

That way we can get around the whole need for changing memory allocation across all the frontends, no? Like the attached.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Attachment

quotes.patch

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Magnus Hagander

Date:

01 January 2013, 20:25:11

On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:

Hi,

now that PQconninfo() was committed, I rebased the subsequent
patches. Actual code change was only in the last patch, to
conform to the committed PQconninfo() API.

I've applied a modified version of the "tar unification" patch to master. It didn't apply cleanly so I ended up redoing a number of the things manually, but the end result is fairly similar. I don't think it'll cause anything but really trivial merge conflicts against the 04 patch, but I didn't actually check that (e.g. it's tarCreateHeader() not _tarCreateHeader() now).

I took at look at the basebackup patch as well. Easier to get now that it's commented :) There's quite a lot of code in there whereby it tries to remove recovery.conf from the tar stream if it's already in there. That seems like an ugly way to do it. I see two other options, that I think both are better. If we do need it to be removed, we should probably pass that as a parameter up to the walsender, and make sure the file isn't included in the first place. But we can also leave it in there - the tar format is perfectly happy to have multiple copies of the same file in the archive - it'll just use whichever copy comes last.

Given that the code there is already fairly difficult to track, I think just keeping it simpler is definitely worth doing one of those two. I would vote for just leaving two copies of recovery.conf in there.

Comments/thoughts from others?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

01 January 2013, 21:14:10

2013-01-01 18:25 keltezéssel, Magnus Hagander írta:

On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
Hi,

now that PQconninfo() was committed, I rebased the subsequent
patches. Actual code change was only in the last patch, to
conform to the committed PQconninfo() API.

I've applied a modified version of the "tar unification" patch to master. It didn't apply cleanly so I ended up redoing a number of the things manually, but the end result is fairly similar. I don't think it'll cause anything but really trivial merge conflicts against the 04 patch, but I didn't actually check that (e.g. it's tarCreateHeader() not _tarCreateHeader() now).

Thanks.

I took at look at the basebackup patch as well. Easier to get now that it's commented :) There's quite a lot of code in there whereby it tries to remove recovery.conf from the tar stream if it's already in there. That seems like an ugly way to do it. I see two other options, that I think both are better. If we do need it to be removed, we should probably pass that as a parameter up to the walsender, and make sure the file isn't included in the first place. But we can also leave it in there - the tar format is perfectly happy to have multiple copies of the same file in the archive - it'll just use whichever copy comes last.

IIRC, Fujii Masao's wish was to remove the recovery.conf from the tar stream.
I know that the tar format is perfectly happy with two files with the same
path name but his reasoning was that GUIs (like WinRar) may get confused
by two such files and cannot decide which one to extract. I didn't actually
test this but it is a reasonable suspicion.

Passing a parameter to the walsender may be a better solution.
It's a bad idea only because it wasn't mine. ;-)

The only problem with this idea is that currently there's nothing that stops
pg_basebackup to be a generic and backwards-compatible tool.
It simply receives a TAR stream via plain PQgetCopyData() and optionally
extracts it. The client-side solution to skip the recovery.conf file keeps this
backward compatible feature. I tested this and pg_basebackup from 9.3dev
happily backs up a 9.2.2 database...

Given that the code there is already fairly difficult to track, I think just keeping it simpler is definitely worth doing one of those two. I would vote for just leaving two copies of recovery.conf in there.

Comments/thoughts from others?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Best regards,
Zoltán Böszörmény

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Magnus Hagander

Date:

01 January 2013, 21:20:40

On Tue, Jan 1, 2013 at 7:13 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:

2013-01-01 18:25 keltezéssel, Magnus Hagander írta:

On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
Hi,

now that PQconninfo() was committed, I rebased the subsequent
patches. Actual code change was only in the last patch, to
conform to the committed PQconninfo() API.

I've applied a modified version of the "tar unification" patch to master. It didn't apply cleanly so I ended up redoing a number of the things manually, but the end result is fairly similar. I don't think it'll cause anything but really trivial merge conflicts against the 04 patch, but I didn't actually check that (e.g. it's tarCreateHeader() not _tarCreateHeader() now).

Thanks.

I took at look at the basebackup patch as well. Easier to get now that it's commented :) There's quite a lot of code in there whereby it tries to remove recovery.conf from the tar stream if it's already in there. That seems like an ugly way to do it. I see two other options, that I think both are better. If we do need it to be removed, we should probably pass that as a parameter up to the walsender, and make sure the file isn't included in the first place. But we can also leave it in there - the tar format is perfectly happy to have multiple copies of the same file in the archive - it'll just use whichever copy comes last.

IIRC, Fujii Masao's wish was to remove the recovery.conf from the tar stream.
I know that the tar format is perfectly happy with two files with the same
path name but his reasoning was that GUIs (like WinRar) may get confused
by two such files and cannot decide which one to extract. I didn't actually
test this but it is a reasonable suspicion.

Hmm. Somehow, I missed that part of the discussion. Sorry, didn't realize it had been mentioned already.

Passing a parameter to the walsender may be a better solution.
It's a bad idea only because it wasn't mine. ;-)

The only problem with this idea is that currently there's nothing that stops
pg_basebackup to be a generic and backwards-compatible tool.
It simply receives a TAR stream via plain PQgetCopyData() and optionally
extracts it. The client-side solution to skip the recovery.conf file keeps this
backward compatible feature. I tested this and pg_basebackup from 9.3dev
happily backs up a 9.2.2 database...

I thought commit add6c3179a4d4fa3e62dd3e86a00f23303336bac at leasdt broke that? In particular, did you test where any of those values were different between client and server? Or maybe just didn't test in -x mode?

I actually thought there was something else that broke the compatibility, but I can't seem to find it so perhaps that was something that was never committed.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

01 January 2013, 21:26:37

2013-01-01 17:18 keltezéssel, Magnus Hagander írta:

On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
Hi,

now that PQconninfo() was committed, I rebased the subsequent
patches. Actual code change was only in the last patch, to
conform to the committed PQconninfo() API.

So, finally coming back to this one.

What happened to Alvaro's suggestion of:

> It seems simpler to have the escape_quotes utility function in port just
> not use pg_malloc at all, have it return NULL or similar failure
> indicator when malloc() fails, and then the caller decides what to do.

I seem to have skipped it while reading my mails,
I don't remember this suggestion at all. Sorry.

That way we can get around the whole need for changing memory allocation across all the frontends, no? Like the attached.

Sure it's simpler but then the consistent look of the code is lost.

What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Tom Lane

Date:

02 January 2013, 03:25:03

Boszormenyi Zoltan <zb@cybertec.at> writes:
> 2013-01-01 17:18 keltez�ssel, Magnus Hagander �rta:
>> That way we can get around the whole need for changing memory allocation across all the 
>> frontends, no? Like the attached.

> Sure it's simpler but then the consistent look of the code is lost.

> What about the other patch to unify pg_malloc and friends?
> Basically all client code boils down to
>      fprintf(stderr, ...)
> in different disguise in their error reporting, so that patch can
> also be simplified but it seems that the atexit() - either explicitly
> or hidden behind InitPostgresFrontend() - cannot be avoided.

Meh.  I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.
I like Magnus' version a lot better than that idea.

A bigger issue that I notice with this code is that it's only correct in
backend-safe encodings, as the comment mentions.  If we're going to be
putting it into frontend programs, how safe is that going to be?
        regards, tom lane

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

02 January 2013, 11:59:31

2013-01-02 01:24 keltezéssel, Tom Lane írta:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
>> 2013-01-01 17:18 keltezéssel, Magnus Hagander írta:
>>> That way we can get around the whole need for changing memory allocation across all the
>>> frontends, no? Like the attached.
>> Sure it's simpler but then the consistent look of the code is lost.
>> What about the other patch to unify pg_malloc and friends?
>> Basically all client code boils down to
>>       fprintf(stderr, ...)
>> in different disguise in their error reporting, so that patch can
>> also be simplified but it seems that the atexit() - either explicitly
>> or hidden behind InitPostgresFrontend() - cannot be avoided.
> Meh.  I find it seriously wrongheaded that something as minor as an
> escape_quotes() function should get to dictate both malloc wrappers
> and error recovery handling throughout every program that might use it.

Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.

I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.

> I like Magnus' version a lot better than that idea.

OK, I will post the core patch building on his code.

> A bigger issue that I notice with this code is that it's only correct in
> backend-safe encodings, as the comment mentions.  If we're going to be
> putting it into frontend programs, how safe is that going to be?
>
>             regards, tom lane

The question in a different form is: does PostgreSQL support
non-ASCII-safe encodings at all (or on the client side)? Forgive
my ignorance and enlighten me: how many such encodings
exist besides EBCDIC? Is UTF-16 non-ASCII-safe?

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Magnus Hagander

Date:

02 January 2013, 12:12:48

On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:

2013-01-02 01:24 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan <zb@cybertec.at> writes:
2013-01-01 17:18 keltezéssel, Magnus Hagander írta:
That way we can get around the whole need for changing memory allocation across all the
frontends, no? Like the attached.
Sure it's simpler but then the consistent look of the code is lost.
What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.
Meh. I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.

Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.

I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.

Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's good or bad features.

I like Magnus' version a lot better than that idea.

OK, I will post the core patch building on his code.

Thanks.

A bigger issue that I notice with this code is that it's only correct in
backend-safe encodings, as the comment mentions. If we're going to be
putting it into frontend programs, how safe is that going to be?

regards, tom lane

The question in a different form is: does PostgreSQL support
non-ASCII-safe encodings at all (or on the client side)? Forgive
my ignorance and enlighten me: how many such encodings
exist besides EBCDIC? Is UTF-16 non-ASCII-safe?

We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html.

There are quite a few far-eastern encodings that aren't available as server encondings, and I believe it's all for this reason.

That said, do we need to care *in this specific case*? We use it in initdb to parse config strings, I believe. And we'd use it to parse a conninfo string in pg_basebackup, correct? Perhaps all we need to do is to clearly comment that it doesn't work with non-ascii safe encodings, or rename it to indicate that it's limited in this?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

02 January 2013, 12:37:43

2013-01-02 10:12 keltezéssel, Magnus Hagander írta:

On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
2013-01-02 01:24 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan <zb@cybertec.at> writes:
2013-01-01 17:18 keltezéssel, Magnus Hagander írta:
That way we can get around the whole need for changing memory allocation across all the
frontends, no? Like the attached.
Sure it's simpler but then the consistent look of the code is lost.
What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.
Meh. I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.

Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.

I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.

Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's good or bad features.

I like Magnus' version a lot better than that idea.

OK, I will post the core patch building on his code.

Thanks.

A bigger issue that I notice with this code is that it's only correct in
backend-safe encodings, as the comment mentions. If we're going to be
putting it into frontend programs, how safe is that going to be?

regards, tom lane

The question in a different form is: does PostgreSQL support
non-ASCII-safe encodings at all (or on the client side)? Forgive
my ignorance and enlighten me: how many such encodings
exist besides EBCDIC? Is UTF-16 non-ASCII-safe?

We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html.

There are quite a few far-eastern encodings that aren't available as server encondings, and I believe it's all for this reason.

I see, thanks.

That said, do we need to care *in this specific case*? We use it in initdb to parse config strings, I believe. And we'd use it to parse a conninfo string in pg_basebackup, correct?

Correct.

Perhaps all we need to do is to clearly comment that it doesn't work with non-ascii safe encodings, or rename it to indicate that it's limited in this?

If you send a new patch with the function renamed accordingly, I will modify
my code to use it.

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

02 January 2013, 13:54:28

2013-01-02 10:37 keltezéssel, Boszormenyi Zoltan írta:

2013-01-02 10:12 keltezéssel, Magnus Hagander írta:
On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
2013-01-02 01:24 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan <zb@cybertec.at> writes:
2013-01-01 17:18 keltezéssel, Magnus Hagander írta:
That way we can get around the whole need for changing memory allocation across all the
frontends, no? Like the attached.
Sure it's simpler but then the consistent look of the code is lost.
What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.
Meh. I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.

Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.

I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.

Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's good or bad features.

I like Magnus' version a lot better than that idea.

OK, I will post the core patch building on his code.

Thanks.

A bigger issue that I notice with this code is that it's only correct in
backend-safe encodings, as the comment mentions. If we're going to be
putting it into frontend programs, how safe is that going to be?

regards, tom lane

The question in a different form is: does PostgreSQL support
non-ASCII-safe encodings at all (or on the client side)? Forgive
my ignorance and enlighten me: how many such encodings
exist besides EBCDIC? Is UTF-16 non-ASCII-safe?

We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html.

There are quite a few far-eastern encodings that aren't available as server encondings, and I believe it's all for this reason.

I see, thanks.

That said, do we need to care *in this specific case*? We use it in initdb to parse config strings, I believe. And we'd use it to parse a conninfo string in pg_basebackup, correct?

Correct.

Perhaps all we need to do is to clearly comment that it doesn't work with non-ascii safe encodings, or rename it to indicate that it's limited in this?

If you send a new patch with the function renamed accordingly, I will modify
my code to use it.

Attached is the quotes-v2 patch, the function is renamed and
the comment is modified plus the pg_basebackup v21 patch
that uses this function.

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de    http://www.postgresql.at/

Attachment

[PATCH] Factor out pg_malloc and friends into port code

From

Boszormenyi Zoltan

Date:

02 January 2013, 16:00:06

2013-01-02 10:12 keltezéssel, Magnus Hagander írta:

On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
2013-01-02 01:24 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan <zb@cybertec.at> writes:
2013-01-01 17:18 keltezéssel, Magnus Hagander írta:
That way we can get around the whole need for changing memory allocation across all the
frontends, no? Like the attached.
Sure it's simpler but then the consistent look of the code is lost.
What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.
Meh. I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.

Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.

I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.

Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's good or bad features.

The patch is attached. There is no extra atexit() code in this one.

I did this over my pg_basebackup patch, there are two chunks
that gets rejected if applied without it: one in initdb.c, the other is
in src/port/Makefile. It's because the modified codes are too close
to each other.

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de    http://www.postgresql.at/

Attachment

pg_malloc-unified-v9.patch

Re: [PATCH] Make pg_basebackup configure and start standby [Review]

From

Boszormenyi Zoltan

Date:

03 January 2013, 15:34:04

2013-01-02 11:54 keltezéssel, Boszormenyi Zoltan írta:

2013-01-02 10:37 keltezéssel, Boszormenyi Zoltan írta:
2013-01-02 10:12 keltezéssel, Magnus Hagander írta:
On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
2013-01-02 01:24 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan <zb@cybertec.at> writes:
2013-01-01 17:18 keltezéssel, Magnus Hagander írta:
That way we can get around the whole need for changing memory allocation across all the
frontends, no? Like the attached.
Sure it's simpler but then the consistent look of the code is lost.
What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.
Meh. I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.

Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.

I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.

Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's good or bad features.

I like Magnus' version a lot better than that idea.

OK, I will post the core patch building on his code.

Thanks.

A bigger issue that I notice with this code is that it's only correct in
backend-safe encodings, as the comment mentions. If we're going to be
putting it into frontend programs, how safe is that going to be?

regards, tom lane

The question in a different form is: does PostgreSQL support
non-ASCII-safe encodings at all (or on the client side)? Forgive
my ignorance and enlighten me: how many such encodings
exist besides EBCDIC? Is UTF-16 non-ASCII-safe?

We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html.

There are quite a few far-eastern encodings that aren't available as server encondings, and I believe it's all for this reason.

I see, thanks.

That said, do we need to care *in this specific case*? We use it in initdb to parse config strings, I believe. And we'd use it to parse a conninfo string in pg_basebackup, correct?

Correct.

Perhaps all we need to do is to clearly comment that it doesn't work with non-ascii safe encodings, or rename it to indicate that it's limited in this?

If you send a new patch with the function renamed accordingly, I will modify
my code to use it.

Attached is the quotes-v2 patch, the function is renamed and
the comment is modified plus the pg_basebackup v21 patch
that uses this function.

Rebased after pgtar.h was added. The quotes.c comment was modified
even more so it doesn't say this function is used for SQL string literals.

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de    http://www.postgresql.at/

Boszormenyi Zoltan wrote:
> 2013-01-02 10:12 keltezéssel, Magnus Hagander írta:

> ><mailto:zb@cybertec.at>> wrote:

> >    Actually, the unification of pg_malloc and friends wasn't dictated
> >    by this little code, it was just that pg_basebackup doesn't provide
> >    a pg_malloc implementation (only pg_malloc0) that is used by
> >    initdb's escape_quotes() function. Then I noticed how wide these
> >    almost identical functions have spread into client apps already.

> >Please do keep that one separate - let's avoid unnecessary
> >feature-creep, whether it's good or bad features.
>
> The patch is attached. There is no extra atexit() code in this one.

I have applied a patch similar in spirit to this one.

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