Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Make the streaming replication protocol messages architecture-independent.
- add6c3179a4d 9.3.0 cited

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

Amit Kapila <amit.kapila@huawei.com> — 2012-09-20T10:30:47Z

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 
- 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. 
According to me it can be useful for users who have automated scripts to
start server after backup can use this feature. 

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. 
    
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); 

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); 
+        } 

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; 

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. 

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

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 

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


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. 

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. 

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

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. 



 

With Regards,

Amit Kapila.

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-03T16:15:58Z

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/

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

Peter Eisentraut <peter_e@gmx.net> — 2012-10-04T03:24:31Z

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.  The parameters to add to the connection
string should be driven off the authoritative list in PQconninfoOptions.

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-04T04:47:09Z

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.

>    The parameters to add to the connection
> string should be driven off the authoritative list in PQconninfoOptions.

So, should I add a second flag to PQconninfoOption to indicate that
certain options should not be used for primary_conninfo?

Thanks and 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]

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-04T10:42:37Z

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/

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-04T11:17:49Z

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/

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

Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-04T14:43:15Z

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]

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-04T16:19:36Z

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/

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

Amit Kapila <amit.kapila@huawei.com> — 2012-10-10T06:58:36Z

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 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? 

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]

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-10T13:12:15Z

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/

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

Fujii Masao <masao.fujii@gmail.com> — 2012-10-10T16:23:40Z

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

$ 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.


+        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?

Regards,

-- 
Fujii Masao

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-10T18:36:07Z

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.

> $ 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/

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-10T19:58:08Z

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/

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

Fujii Masao <masao.fujii@gmail.com> — 2012-10-11T00:02:46Z

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.

>> +        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?

There is no restriction that the backup which was taken by using
pg_basebackup -x cannot be used to start the standby. So I wonder
why -R option cannot work together with -x. It's useful if recovery.conf
is automatically written even with pg_basebackup -x.

And I found another problem: when -(stdout) is specified in -D option,
recovery.conf fails to be written.

$ pg_basebackup -D- -F t -c fast -R > hoge.tar
NOTICE:  WAL archiving is not enabled; you must ensure that all
required WAL segments are copied through other means to complete the
backup
pg_basebackup: cannot create -/recovery.conf: No such file or directory

Regards,

-- 
Fujii Masao

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

Fujii Masao <masao.fujii@gmail.com> — 2012-10-11T00:04:54Z

On Thu, Oct 11, 2012 at 4:58 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> 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.

Thanks. Will test.

Regards,

-- 
Fujii Masao

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

Robert Haas <robertmhaas@gmail.com> — 2012-10-12T20:21:56Z

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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-14T11:41:07Z

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/

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-14T15:57:10Z

2012-10-11 02:02 keltezéssel, Fujii Masao írta:
> 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.

It's implemented now.

>>> +        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?
> There is no restriction that the backup which was taken by using
> pg_basebackup -x cannot be used to start the standby. So I wonder
> why -R option cannot work together with -x. It's useful if recovery.conf
> is automatically written even with pg_basebackup -x.

There was a problem with 9.0.x (maybe even with 9.1) that the backup
failed to come up as a standby if -x or -X was specified. I don't know
if it was a bug, limitation or intended behaviour.

Before removing the check for conflicting options, I would like to ask:
is there such a known conflict with --xlog-method=stream?

> And I found another problem: when -(stdout) is specified in -D option,
> recovery.conf fails to be written.
>
> $ pg_basebackup -D- -F t -c fast -R > hoge.tar
> NOTICE:  WAL archiving is not enabled; you must ensure that all
> required WAL segments are copied through other means to complete the
> backup
> pg_basebackup: cannot create -/recovery.conf: No such file or directory

Now it works with recovery.conf written into the tar itself. I also tried

$ pg_basebackup -D- -Ft -R

and the directory named "-" was created and of course the recovery.conf
inside it. Is this the intended behaviour regarding "stdout is to be treated
as a directory"?

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]

Fujii Masao <masao.fujii@gmail.com> — 2012-10-14T16:02:20Z

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.

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.

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?

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?

+        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.

I've not reviewed PQconninfo patch yet. Will review.

Regards,

-- 
Fujii Masao

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

Fujii Masao <masao.fujii@gmail.com> — 2012-10-14T16:10:55Z

On Mon, Oct 15, 2012 at 12:57 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> 2012-10-11 02:02 keltezéssel, Fujii Masao írta:
>
>> 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.
>
>
> It's implemented now.

Thanks a lot!

>>>> +        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?
>>
>> There is no restriction that the backup which was taken by using
>> pg_basebackup -x cannot be used to start the standby. So I wonder
>> why -R option cannot work together with -x. It's useful if recovery.conf
>> is automatically written even with pg_basebackup -x.
>
>
> There was a problem with 9.0.x (maybe even with 9.1) that the backup
> failed to come up as a standby if -x or -X was specified. I don't know
> if it was a bug, limitation or intended behaviour.

It sounds a bug to me. It's helpful if you provide the self-contained test case.

> Before removing the check for conflicting options, I would like to ask:
> is there such a known conflict with --xlog-method=stream?

AFAIK, No, we can use the backup which pg_basebackup --xlog-method=stream
took, to start the standby. BTW, --xlog-method=stream cannot be specified
together with -F tar.

>> And I found another problem: when -(stdout) is specified in -D option,
>> recovery.conf fails to be written.
>>
>> $ pg_basebackup -D- -F t -c fast -R > hoge.tar
>> NOTICE:  WAL archiving is not enabled; you must ensure that all
>> required WAL segments are copied through other means to complete the
>> backup
>> pg_basebackup: cannot create -/recovery.conf: No such file or directory
>
>
> Now it works with recovery.conf written into the tar itself. I also tried
>
> $ pg_basebackup -D- -Ft -R
>
> and the directory named "-" was created and of course the recovery.conf
> inside it. Is this the intended behaviour regarding "stdout is to be treated
> as a directory"?

Yes. Thanks.

Regards,

-- 
Fujii Masao

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-14T16:41:13Z

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.

> 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.

> 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?

> 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]

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-14T20:23:48Z

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/

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-14T20:26:07Z

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]

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-14T20:31:10Z

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/

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-10-15T07:46:19Z

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/

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

Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-23T14:52:55Z

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.

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

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

Magnus Hagander <magnus@hagander.net> — 2012-10-23T15:08:56Z

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.

/Magnus

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

Magnus Hagander <magnus@hagander.net> — 2012-11-18T16:20:20Z

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..

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.

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?

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"?

Then, about the pg_basebackup patch:

What's the reason for the () around 512 for TARCHUNKSZ?

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).

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.

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.

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.

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.

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?

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.

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.

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

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

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-11-18T19:24:25Z

Hi,

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.

No problem, thanks for looking at it.

> 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..

You are right, I will change this accordingly.

> 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.

This needs opinions from more people, I am not the one to decide it.
The code would be definitely cleaner without processing this extra
non-1:1 mapping.

> 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?

Yes, and set it to NULL too. Otherwise there might be a case when
the free() leaves a stale pointer value if the extra mapping fails
the strcmp() check. This is all unnecessary if the extra mapping
for requiressl=1 is removed, the code would be straight.

> 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"?

I am not attached to this variable name, I will change it.

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

It's simply a habit, to not forget it for more complex macros.

> 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).

Yes, I left 5 pieces of the hardcoded value of 512, because
I (maybe erroneously) distinguished between a file header
and file "chunks" inside a TAR file, they are all 512.

Is it okay to change every hardcoded 512 to TARCHUNKSZ,
maybe adding a comment to the #define that it must not
be modified ever?

> 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.

Will change it.

> 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.

OK.

> 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.
>
> 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.
>
> 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 copied the tar stuff from bin/pg_dump/pg_backup_tar.c,
so there are at least two copies of this already.
I will look into unifying them.

> 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.

OK. I can copy it, too. ;-)

> 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.

OK.

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

No problem. I will try to update the patches according to
your comments as soon as possible.

Thanks and 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/

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-11-20T16:03:59Z

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/

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-11-20T16:14:57Z

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]

Boszormenyi Zoltan <zb@cybertec.at> — 2012-11-20T19:32:56Z

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/

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-11-21T10:35:00Z

2012-11-20 20:32 keltezéssel, Boszormenyi Zoltan írta:
> 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.

I experimented with unifying escape_quotes() shortly.

The problem is that it calls pg_malloc() which is an executable-specific
function. Some of the bin/* executables define it as calling exit(1)
when malloc() returns NULL, some call it with exit(EXIT_FAILURE)
which happens to be 1 but still can be different from the constant 1.
Some executables only define pg_malloc0() but not pg_malloc().

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.

To unify escape_quotes(), pg_malloc() need to be also unified.
But the diverse requirements for pg_malloc() from different
executables means that both the escape_quotes() and the pg_malloc()
interface need to be extended with the exit function they have to call
and the argument to pass to the exit function. Unless we don't care
about the bug reports about "unexpected EOF from client" messages.

Frankly, it doesn't worth the effort. Let's just keep the two separate
copies of escape_quotes().

BTW, it seems to me that this "unify even the least used functions"
mentality was not considered to be a great feature during the
introduction of pg_malloc(), which is used in far more code than
the TAR functions.

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]

Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-21T13:19:44Z

Boszormenyi Zoltan wrote:

> The problem is that it calls pg_malloc() which is an executable-specific
> function. Some of the bin/* executables define it as calling exit(1)
> when malloc() returns NULL, some call it with exit(EXIT_FAILURE)
> which happens to be 1 but still can be different from the constant 1.
> Some executables only define pg_malloc0() but not pg_malloc().

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.

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

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-11-21T14:12:21Z

2012-11-21 14:19 keltezéssel, Alvaro Herrera írta:
> Boszormenyi Zoltan wrote:
>
>> The problem is that it calls pg_malloc() which is an executable-specific
>> function. Some of the bin/* executables define it as calling exit(1)
>> when malloc() returns NULL, some call it with exit(EXIT_FAILURE)
>> which happens to be 1 but still can be different from the constant 1.
>> Some executables only define pg_malloc0() but not pg_malloc().
> 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.

$ find . -name "*.c" | xargs grep pg_malloc | grep -v "^pg_malloc" | wc -l
277

Too much work for little gain.

Also:
- pg_upgrade calls pg_log(PG_FATAL, ...)
- pgbench logs a line then calls exit(1)

What I can imagine is to introduce a new src/port/ function,
InitPostgresFrontend() or something like that. Every executable
then calls this function first inside their main() it they want to use
pg_malloc() from libpgport.a. This function would accept a pointer
to a structure, and the  struct contains the pointer to the exit
function and the argument too call it with. Other data for different
use cases can be added later if needed.

This way, the pg_malloc() and friends can be unified and their call
interfaces don't have to change. InitPostgresFrontend() needs
to be added to only 9 places instead of changing 277 callers of
pg_malloc() or pg_malloc0().

BTW, the unified pg_malloc() and friends must be inside
#ifdef FRONTEND ... #endif to not leak into the backend code.

Opinions?

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]

Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-21T14:28:31Z

Boszormenyi Zoltan wrote:
> 2012-11-21 14:19 keltezéssel, Alvaro Herrera írta:
> >Boszormenyi Zoltan wrote:
> >
> >>The problem is that it calls pg_malloc() which is an executable-specific
> >>function. Some of the bin/* executables define it as calling exit(1)
> >>when malloc() returns NULL, some call it with exit(EXIT_FAILURE)
> >>which happens to be 1 but still can be different from the constant 1.
> >>Some executables only define pg_malloc0() but not pg_malloc().
> >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.
> 
> $ find . -name "*.c" | xargs grep pg_malloc | grep -v "^pg_malloc" | wc -l
> 277
> 
> Too much work for little gain.

I probably wrote the above in a confusing way.  I am not suggesting that
pg_malloc is changed in any way.

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

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

Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-21T14:29:02Z

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

			regards, tom lane

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-11-21T14:30:54Z

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

Aww, crap. I knew I forgot about something. :-)
Thanks.

>
> 			regards, tom lane
>


-- 
----------------------------------
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]

Boszormenyi Zoltan <zb@cybertec.at> — 2012-11-21T17:39:58Z

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/

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-11-22T10:06:52Z

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/

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-11-22T16:57:01Z

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/

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

Boszormenyi Zoltan <zb@cybertec.at> — 2012-11-30T09:13:27Z

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/

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

Magnus Hagander <magnus@hagander.net> — 2013-01-01T16:18:34Z

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/

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

Magnus Hagander <magnus@hagander.net> — 2013-01-01T17:25:03Z

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]

Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-01T18:13:55Z

2013-01-01 18:25 keltezéssel, Magnus Hagander írta:
>
> On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan <zb@cybertec.at 
> <mailto: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]

Magnus Hagander <magnus@hagander.net> — 2013-01-01T18:20:34Z

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]

Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-01T18:26:14Z

2013-01-01 17:18 keltezéssel, Magnus Hagander írta:
>
> On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan <zb@cybertec.at 
> <mailto: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]

Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-02T00:24:51Z

Boszormenyi Zoltan <zb@cybertec.at> writes:
> 2013-01-01 17:18 keltezssel, 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]

Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-02T08:59:19Z

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]

Magnus Hagander <magnus@hagander.net> — 2013-01-02T09:12:44Z

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]

Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-02T09:37:21Z

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

Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-02T10:54:07Z

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 
>> <mailto:zb@cybertec.at>> wrote:
>>
>>     2013-01-02 01:24 keltezéssel, Tom Lane írta:
>>
>>         Boszormenyi Zoltan <zb@cybertec.at <mailto: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/

[PATCH] Factor out pg_malloc and friends into port code

Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-02T12:59:50Z

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

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

Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-03T12:33:46Z

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 
>>> <mailto:zb@cybertec.at>> wrote:
>>>
>>>     2013-01-02 01:24 keltezéssel, Tom Lane írta:
>>>
>>>         Boszormenyi Zoltan <zb@cybertec.at <mailto: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/

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

Magnus Hagander <magnus@hagander.net> — 2013-01-05T14:41:57Z

On Thu, Jan 3, 2013 at 1:33 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> 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:
> 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.

Quotes patch applied with a few small changes:
1) Support for the msvc build (needs an entry added for new files that
go in src/port if they should be used on Windows)
2) Makefile entries should be alphabetical (yes, that's really trivial
nitpicking :D)


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

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

Magnus Hagander <magnus@hagander.net> — 2013-01-05T15:58:01Z

On Sat, Jan 5, 2013 at 3:41 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Thu, Jan 3, 2013 at 1:33 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>> 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:
>> 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.
>
> Quotes patch applied with a few small changes:
> 1) Support for the msvc build (needs an entry added for new files that
> go in src/port if they should be used on Windows)
> 2) Makefile entries should be alphabetical (yes, that's really trivial
> nitpicking :D)

After some further modifications, I've applied the pg_basebackup patch as well.

Thanks! And again, apologies for dragging the process out longer than
should've been necessary.

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

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

Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-05T16:17:03Z

2013-01-05 16:58 keltezéssel, Magnus Hagander írta:
> On Sat, Jan 5, 2013 at 3:41 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Thu, Jan 3, 2013 at 1:33 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>>> 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:
>>> 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.
>> Quotes patch applied with a few small changes:
>> 1) Support for the msvc build (needs an entry added for new files that
>> go in src/port if they should be used on Windows)
>> 2) Makefile entries should be alphabetical (yes, that's really trivial
>> nitpicking :D)
> After some further modifications, I've applied the pg_basebackup patch as well.

Thank you very much!

> Thanks! And again, apologies for dragging the process out longer than
> should've been necessary.

I blamed it on me not having done reviews earlier in the commitfest,
which I finally did last week. Now, if only Tom could find some time
to review the lock_timeout patch... ;-)

Thanks again and 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] Factor out pg_malloc and friends into port code

Alvaro Herrera <alvherre@2ndquadrant.com> — 2013-02-12T15:14:25Z

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