Re: pgsql: Fix pg_basebackup with in-place tablespaces.

[David Steele]

On 3/16/22 13:33, Thomas Munro wrote:



It seems that the warning at line 8313 is essentially dead code now. I
don't expect test code to have an impact on production systems, even if
the effect is minor.


It's not dead, it's how we'd report something like EACCES or EIO.  Why
we only warn and press on, I'm not sure.  (I'm also looking into why
we ignore OS errors for pgwin32_is_junction everywhere.)


I'm also wondering why this is only a warning.


I'm less worried about what happens when the flag is flipped on then off
because that's not likely to happen in production.


So, concretely, if there is a non-symlink with a name that looks like
an OID under pg_tblspc, previously we'd barf (or apparently press on
with an empty pathname on Windows, which might indicate a lack of
error checking somewhere).  Given the policy of quietly ignoring
anything else in the directory, is it really this code's job to sanity
check the cluster layout?  Hmm, I guess we could fix the problem on
Windows in a different way so that it behaves like Unix, and then
revert this.  You'd get an ignorable not-a-symlink warning as before
(and now it'd work on Windows) when backing up in-place tablespaces,
but I'm not sure it's really an improvement.


I'm not going to press on this, but FWIW this solution sounds better to 
me. Either way it is a behavioral change. Windows used to error in this 
case and now it does not, Unix used to warn in this case and now it does 
not.


For my 2C I'd rather this was a hard error because that makes life a lot 
easier down the line. But, that's certainly not the responsibility of 
this patch.


Regards,
-David

Re: pgsql: Fix pg_basebackup with in-place tablespaces.

[David Steele]

On 3/15/22 23:42, Kyotaro Horiguchi wrote:

At Wed, 16 Mar 2022 11:13:53 +1300, Thomas Munro  wrote 
in

On Wed, Mar 16, 2022 at 10:28 AM Tom Lane  wrote:

David Steele  writes:

On 3/14/22 19:31, Thomas Munro wrote:

Fix pg_basebackup with in-place tablespaces.



Perhaps I'm being picky, but seems like this logic should be wrapped in:
if (allow_in_place_tablespaces)
{
  <...>
}
I worry about strange effects when this GUC is not enabled.


What would happen if someone created an in-place tablespace and
then turned off the GUC?


Then it would break with unhelpful error messages.  I suppose we could
error out explicitly, "in-place tablespace detected, but
allow_in_place_tablespaces not enabled".  I'm not sure why we should
suddenly choose to enforce that *here* though.


+1. The GUC is only to prevent non-developer users from accidentally
creating in-place tablespaces that is not officieally suported.  We
have done nothing about in-place tablespaces other than the things
needed to perform regression test, and I think we won't do that in
future.


Sure, but there is a behavioral change whether the GUC is enabled or 
not. Before, if there was clutter in pg_tblspc there would at least be a 
warning in the log. Now that logging does not happen.


It seems that the warning at line 8313 is essentially dead code now. I 
don't expect test code to have an impact on production systems, even if 
the effect is minor.


I'm less worried about what happens when the flag is flipped on then off 
because that's not likely to happen in production.


Regards,
-David

Re: pgsql: Fix pg_basebackup with in-place tablespaces.

[David Steele]

On 3/14/22 19:31, Thomas Munro wrote:

Fix pg_basebackup with in-place tablespaces.

Previously, pg_basebackup from a cluster that contained an 'in-place'
tablespace, as introduced by commit 7170f215, would produce a harmless
warning on Unix and fail completely on Windows.


Perhaps I'm being picky, but seems like this logic should be wrapped in:

if (allow_in_place_tablespaces)
{
<...>
}

I worry about strange effects when this GUC is not enabled.

Regards,
-David

Re: [COMMITTERS] pgsql: Mark pg_start_backup and pg_stop_backup as parallel-restricted.

[David Steele]

On 6/25/20 11:02 AM, Magnus Hagander wrote:
On Thu, Jun 25, 2020 at 4:56 PM David Steele <mailto:da...@pgmasters.net>> wrote:


So we added that to session initialization in pgBackRest:


https://github.com/pgbackrest/pgbackrest/commit/ea04ec7b3f4c6cf25c1b827c25c6a3c5896159a8

Personally I would've done it *just* for 9.6 and not for 10+, but that's 
mostly semantic :) But if you do it for 9.6 then *eventually* you will 
be able to retire it.


We still support 8.3 so the day when we drop 9.6 support seems so far 
away that I'm just not worried about it.


And yes, we still see users with 8.3/8.4 databases. Mostly they just 
want to update but backups are an essential part of that process and 
some of them are multi-TB databases.


We are planning to drop 8.3 support soon, though, and just tell users to 
go back and use X version of pgbackrest if they really need it. This has 
more to do with the availability of packages than anything else. 
Christoph has built packages all the way back to 8.4 for all recent 
Debian distros (thanks Christoph!) but the only place we can get 8.3 
packages is on EOL distros, e.g. Ubuntu 12.04.



I'm worried that (as Tom said) the planner might find another reason to
choose a parallel query.

I'm looking at this as more than a fix for 9.6. I can't see any reason
for the backup control session to run queries in parallel and possibly
use more resources, so parallelism is disabled for all versions that
support it.

Right. But since the parameters are flagged as parallel restricted in 10+...

Or are you saying you're worried about other things, completely 
unrelated to start/stop backup, that the session might run?


That's what I'm worried about. We run other queries and functions 
besides pg_start_backup()/pg_stop_backup(). None of them need to be 
parallelized so why not just disable it? One less variable to worry about.


Regards,
--
-David
da...@pgmasters.net

Re: [COMMITTERS] pgsql: Mark pg_start_backup and pg_stop_backup as parallel-restricted.

[David Steele]

On 6/25/20 8:43 AM, Magnus Hagander wrote:
On Thu, Jun 25, 2020 at 2:11 PM David Steele <mailto:da...@pgmasters.net>> wrote:

On 6/24/20 6:27 PM, Tom Lane wrote:
 >
 > I was able to force it like this:
 >
 > regression=# set force_parallel_mode TO 'on';
 > SET

Ah, yes, that works. Now at least I can test it.

 > It doesn't seem terribly likely that anybody would be using
 > force_parallel_mode = on in production, but perhaps there's some
 > reasonable combination of the other parallelization planning GUCs
 > that can make this plan look attractive.

I'll also ask the user if they have this GUC enabled.


The user confirmed they are running with force_parallel_mode=on so that 
probably explains why we have never seen this in the field before.


Maybe have pgbackrest issue an explicit SET force_parallel_mode=off when 
it runs against a 9.6?


According to the documentation the way to disable parallelism is:

set max_parallel_workers_per_gather = 0

So we added that to session initialization in pgBackRest:

https://github.com/pgbackrest/pgbackrest/commit/ea04ec7b3f4c6cf25c1b827c25c6a3c5896159a8

I'm worried that (as Tom said) the planner might find another reason to 
choose a parallel query.


I'm looking at this as more than a fix for 9.6. I can't see any reason 
for the backup control session to run queries in parallel and possibly 
use more resources, so parallelism is disabled for all versions that 
support it.


Regards,
--
-David
da...@pgmasters.net

Re: [COMMITTERS] pgsql: Mark pg_start_backup and pg_stop_backup as parallel-restricted.

[David Steele]

On 6/24/20 6:27 PM, Tom Lane wrote:

David Steele  writes:

... So apparently it is possible. To get them working as soon as possible I
recommended that they run:
alter role postgres set max_parallel_workers_per_gather = 0;
And that solved their problem. 9.6 is getting on in years so I'm not
sure how much time/effort we want to spend on this, but I figured it was
worth mentioning.



I did another round of trying to reproduce the issue but came up short a
second time.


I was able to force it like this:

regression=# set force_parallel_mode TO 'on';
SET


Ah, yes, that works. Now at least I can test it.


It doesn't seem terribly likely that anybody would be using
force_parallel_mode = on in production, but perhaps there's some
reasonable combination of the other parallelization planning GUCs
that can make this plan look attractive.


I'll also ask the user if they have this GUC enabled.


I'm kind of inclined not to bother with a code fix at this late date,
given that we've had so few trouble reports.  The right answer is
to tell anyone who's affected to fix their catalogs manually, viz

update pg_proc set proparallel = 'r' where proname = 'pg_start_backup';
update pg_proc set proparallel = 'r' where proname = 'pg_stop_backup';


Only pg_stop_backup() needs to be updated, but that's probably the best 
solution.



If we had back-patched 9fe3c644a (sans catversion bump of course)
at the time, then initdb's done with 9.6.3 or later would have gotten
this right.  In hindsight, not doing that was clearly wrong.  But it
seems a bit late to do it now.


OK by me.

Regards,
--
-David
da...@pgmasters.net

Re: [COMMITTERS] pgsql: Mark pg_start_backup and pg_stop_backup as parallel-restricted.

[David Steele]

On 3/6/17 3:28 PM, Stephen Frost wrote:


* Tom Lane (t...@sss.pgh.pa.us) wrote:

David Steele  writes:

On 3/6/17 12:48 PM, Robert Haas wrote:

This issue also exists in 9.6, but we obviously can't do anything
about 9.6 clusters that already exist.  Possibly this could be
back-patched so that future 9.6 clusters would come out OK, or
possibly we should back-patch some other fix, but that would need more
discussion.



I think it would be worth back-patching the catalog fix for future 9.6
clusters as a start.


Yes, I think it's rather silly not to do so.  We have made comparable
backpatched fixes multiple times in the past.  What is worth discussing is
whether there are *additional* things we ought to do in 9.6 to prevent
misbehavior in installations initdb'd pre-9.6.3.

If there's a cheap way of testing "AmInParallelWorker", I'd be in favor of
adding a quick-n-dirty test and ereport(ERROR) to these functions in the
9.6 branch, so that at least you get a clean error and not some weird
misbehavior.  Not sure if there's anything more we can do than that.


That's more-or-less what I was thinking (and suggested to David over IM
a little while ago, actually).  I don't know if there's an easy way to
do such a check, but I don't think it would really need to be
particularly cheap, just not overly complex.  These code paths are
certainly not ones that need to be high-performance.


Way back when, I tried to get backups on 9.6 to fail due to 
pg_stop_backup() running in a parallel worker and I was not able to make 
it happen so I gave up and moved on to other things.


However, we just got a report from the field that a user ran into this 
exact situation:


ERROR: [057]: raised from remote-0 protocol on 'XX.XX.XXX.XXX': unable 
to execute query 'select lsn::text as lsn,

pg_catalog.pg_xlogfile_name(lsn)::text as wal_segment_name,
labelfile::text as backuplabel_file,
spcmapfile::text as tablespacemap_file
from pg_catalog.pg_stop_backup(false)'
ERROR: non-exclusive backup is not in progress
HINT: Did you mean to use pg_stop_backup('t')?
CONTEXT: parallel worker

https://github.com/pgbackrest/pgbackrest/issues/1083

So apparently it is possible. To get them working as soon as possible I 
recommended that they run:


alter role postgres set max_parallel_workers_per_gather = 0;

And that solved their problem. 9.6 is getting on in years so I'm not 
sure how much time/effort we want to spend on this, but I figured it was 
worth mentioning.


I did another round of trying to reproduce the issue but came up short a 
second time.


I'm willing to put together a patch for 9.6 to update the catalog and/or 
add the error if there is interest.


Thoughts?

Regards,
--
-David
da...@pgmasters.net

Re: pgsql: Implement waiting for given lsn at transaction start

[David Steele]

On 4/7/20 7:32 PM, Alexander Korotkov wrote:

On Wed, Apr 8, 2020 at 1:34 AM Anna Akenteva  wrote:

On 2020-04-08 00:45, David G. Johnston wrote:


While there is lots of discussion it ended up with the thread at
"returned with feedback" (a month ago) and now we have a commit.
There seems to be other relevant discussion not linked to.

David J.


Hello! Sorry about the confusion.

This feature somehow managed to have multiple separate discussion
threads:
[1]
https://postgr.es/m/0240c26c-9f84-30ea-fca9-93ab2df5f305%40postgrespro.ru
[2]
https://postgr.es/m/80f267591b373db5588d580fdfb432f0%40postgrespro.ru
[3]
https://postgr.es/m/195e2d07ead315b1620f1a053313f490%40postgrespro.ru


My email client somehow merge these threads into single one.  This is
why I missed [2] and [3] in the commit message.  Sorry for that.


Well, I think Ivan should have certainly replied on the original thread 
(where I marked the patch RwF after Fujii's recommendation) before 
detaching it and reviving the patch.


Better yet, the original thread should not have been detached at all.

Now having read the active thread it seems this patch was pushed through 
at the last moment despite some objections from Amit.


I can see there are new proposals for syntax post-commit on the thread.

Honestly, I'm at a loss for what to say. This just seems wrong.

Regards,
--
-David
da...@pgmasters.net

Re: pgsql: Avoid -Wconversion warnings when using checksum_impl.h

[David Steele]

On 3/5/20 8:10 PM, Michael Paquier wrote:

On Thu, Mar 05, 2020 at 07:58:50PM -0500, David Steele wrote:

Yeah, keeping BLKSZ a constant is pretty important for performance. That's
one of the main reasons that we only support the default.


Doing something is not complicated, now how would people like to do
it?  Here are the options I can think of:
1) A TAP test, which bypasses the tests if the page size is not 8k.
2) A test for src/test/regress/, with a method similar to what we do
for collate.sql for the compilations without ICU.

In both cases mentioned here, we would need a SQL function to handle
the work.


Couldn we use pageinspect?

--
-David
da...@pgmasters.net

Re: pgsql: Avoid -Wconversion warnings when using checksum_impl.h

[David Steele]

On 3/5/20 7:48 PM, Michael Paquier wrote:

On Thu, Mar 05, 2020 at 04:14:20PM -0800, Magnus Hagander wrote:

That's a pretty dangerous mistake, and moreso because we don't have a
test around it. We should really find a way to add such a test -- just
a hardcoded page calculation that should always return the same value
perhaps?


Yes..  Using pg_checksums --check on an upgraded instance which had
checksums enabled would detect that immediately, but it could be
possible also to have a SQL-callable function which takes in input a
bytea and returns the checksum.  In order to make such tests reliable
with any page size, we could pass down the page size to
pg_checksum_page(), and then give the function's caller this
possibility.  However I recall that we'd rather keep BLCKSZ hardcoded
in checksum_impl.h on performance grounds.


Yeah, keeping BLKSZ a constant is pretty important for performance. 
That's one of the main reasons that we only support the default.


Regards,
--
-David
da...@pgmasters.net

Re: pgsql: Avoid -Wconversion warnings when using checksum_impl.h

[David Steele]

On 3/5/20 6:30 PM, Michael Paquier wrote:

On Thu, Mar 05, 2020 at 12:51:50PM -0500, David Steele wrote:


Bit of a bummer that this passed make check-world, but the pgBackRest tests
definitely did not like it.


Is that because you have a copy of this routine in your code?


Yes, we pull this file into pgBackRest for our checksum validation.  We 
also do unit and integration tests against it.


Regards,
--
-David
da...@pgmasters.net

Re: pgsql: Avoid -Wconversion warnings when using checksum_impl.h

[David Steele]

On 3/5/20 7:14 PM, Magnus Hagander wrote:

On Thu, Mar 5, 2020 at 3:30 PM Michael Paquier  wrote:


On Thu, Mar 05, 2020 at 12:51:50PM -0500, David Steele wrote:

Looks like you changed 65535 -> 65536 before commit.  I checked the original
patch and it has 65535.


That's my fault, thanks.  I have been playing with the surroundings
while looking at your patch.


:/

That's a pretty dangerous mistake, and moreso because we don't have a
test around it. We should really find a way to add such a test -- just
a hardcoded page calculation that should always return the same value
perhaps?


FWIW, we use static values in our unit tests (which are written in C), 
then test against packaged versions of Postgres for integration tests.


When I saw the commit I pulled it in so I could remove instructions for 
the manual step to add the cast.  So in this case the issue was apparent 
really quickly.  Normally we only pull in new code from PostgreSQL once 
a year.


We think our unit tests against static values may have endianess issues 
but we have not verified that one way or the other.  Here's what they 
look like:


https://github.com/pgbackrest/pgbackrest/blob/e55443c890181ea63a350275447885331c8254e4/test/src/module/postgres/interfaceTest.c#L182

Regards,
--
-David
da...@pgmasters.net

Re: pgsql: Avoid -Wconversion warnings when using checksum_impl.h

[David Steele]

Hi Michael,

On 3/5/20 12:13 AM, Michael Paquier wrote:

Avoid -Wconversion warnings when using checksum_impl.h

This does not matter much when compiling Postgres proper as many
warnings exist when enabling this compilation flag, but it can be
annoying for external modules willing to use both.


Looks like you changed 65535 -> 65536 before commit.  I checked the 
original patch and it has 65535.


Bit of a bummer that this passed make check-world, but the pgBackRest 
tests definitely did not like it.


Thanks,
--
-David
da...@pgmasters.net

Re: pgsql: doc: Further clarify how recovery target parameters are applied

[David Steele]

On 11/6/19 4:49 AM, Fujii Masao wrote:

On Wed, Nov 6, 2019 at 4:29 PM Peter Eisentraut
 wrote:


On 2019-11-06 05:48, Fujii Masao wrote:

Patch attached. As I argued upthread, IMO it's better to remove
the latter description from the doc and the patch does that.
Also the patch adds "mainly" into the former description.


I think we should list explicitly what is applied and what is not.  This
is the reference documentation after all.


Or it might be better to add something like "This setting has no effect if
in standby mode / archive recovery" into the description of each parameter,
instead.


+1.  This will probably be easier to maintain and less confusing overall.

--
-David
da...@pgmasters.net

Re: pgsql: Make crash recovery ignore recovery target settings.

[David Steele]

On 9/29/19 11:02 PM, Fujii Masao wrote:
> On Mon, Sep 30, 2019 at 11:45 AM Michael Paquier  wrote:
>>
>> Hi Fujii-san,
>>
>> On Mon, Sep 30, 2019 at 01:21:00AM +, Fujii Masao wrote:
>>> Make crash recovery ignore recovery target settings.
>>>
>>> In v11 or before, recovery target settings could not take effect in
>>> crash recovery because they are specified in recovery.conf and
>>> crash recovery always starts without recovery.conf. But commit
>>> 2dedf4d9a8 integrated recovery.conf into postgresql.conf and
>>> which unexpectedly allowed recovery target settings to take effect
>>> even in crash recovery. This is definitely not good behavior.
>>>
>>> To fix the issue, this commit makes crash recovery always ignore
>>> recovery target settings.
>>>
>>> Back-patch to v12.
>>
>> It would be nice to avoid committing stuff into REL_12_STABLE until
>> the version is tagged (not stamped!) as the tarball wrap for the GA of
>> this week is going to be soon.
> 
> I was thinking this is open item for v12 and needs to be fixed before GA.
> But per discussion in pgsql-release, the consensus seems to be to
> ship GA first and fix that after GA. Sorry for my inattention...
> I suspend to commit other recovery-related things until GA is tagged.

I'm surprised that we would consider going to GA with an issue like this
outstanding.

-- 
-David
da...@pgmasters.net

Re: pgsql: Make WAL segment size configurable at initdb time.

[David Steele]

On 11/9/18 10:30 PM, Andres Freund wrote:
> On 2018-11-09 21:45:18 -0500, David Steele wrote:
>> On 10/5/18 1:03 PM, David Steele wrote:
>>>
>>> So, while the WAL segment size used to be expressed in terms of 8K pages
>>> it is now expressed in terms of absolute bytes.  This seemed to me to be
>>> a very deliberate change in the original commit so I guessed it was done
>>> for clarity, but that the docs didn't get the message.
>>
>> Thoughts on this?
>>
>> I know it's minor in the grand scheme of things but it caused me some
>> confusion when I was updating pgBackRest for v11 and I imagine it might
>> do the same for others.
> 
> Sorry for forgetting this, pushed.

No worries.  Thanks for pushing it.

> I kinda wonder whether we should move a few GUCs out of the
>
> section. Or adapt its description. Because a significant portion of its
> contents don't accurately seem to be described by
>  The following parameters are read-only, and are determined
>  when PostgreSQL is compiled or when it is
>  installed.
> right?

I agree that "installed" is a bit vague.

Most of them are set at compile-time, some are set at initdb, and a few
are set at CREATE DATABASE.

I'm not sure about moving any out since it seems like "presets" fits the
bill.  Maybe just break it up into three sections?

-- 
-David
da...@pgmasters.net

Re: pgsql: Make WAL segment size configurable at initdb time.

[David Steele]

On 10/5/18 1:03 PM, David Steele wrote:
> Hi Andres,
> 
> On 10/5/18 5:54 PM, Andres Freund wrote:
>> On 2018-09-20 11:48:08 -0400, David Steele wrote:
>>
>>> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
>>> index e1073ac6d3..3bfd172441 100644
>>> --- a/doc/src/sgml/config.sgml
>>> +++ b/doc/src/sgml/config.sgml
>>> @@ -8440,10 +8440,8 @@ dynamic_library_path =
>>> 'C:\tools\postgresql;H:\my_project\lib;$libdir'
>>>     
>>>     
>>>  
>>> -    Reports the number of blocks (pages) in a WAL segment file.
>>> -    The total size of a WAL segment file in bytes is equal to
>>> -    wal_segment_size multiplied by
>>> wal_block_size;
>>> -    by default this is 16MB.  See >> linkend="wal-configuration"/> for
>>> +    Reports the size of write ahead log segments.
>>> +    The default value is 16MB. See >> linkend="wal-configuration"/> for
>>>   more information.
>>>  
>>>     
>>
>> Why is this actually more correct? You mean because we have a conversion
>> that does the mb conversion at display time?
> 
> In pre-11 versions of Postgres, you get this:
> 
> postgres=# select setting, unit from pg_settings where name =
> 'wal_segment_size';
>  setting | unit
> -+--
>  2048    | 8kB
> 
> But in v11 you get this:
> 
> select setting, unit from pg_settings where name = 'wal_segment_size';
>  setting  | unit
> --+--
>  16777216 | B
> 
> So, while the WAL segment size used to be expressed in terms of 8K pages
> it is now expressed in terms of absolute bytes.  This seemed to me to be
> a very deliberate change in the original commit so I guessed it was done
> for clarity, but that the docs didn't get the message.

Thoughts on this?

I know it's minor in the grand scheme of things but it caused me some
confusion when I was updating pgBackRest for v11 and I imagine it might
do the same for others.

-- 
-David
da...@pgmasters.net

Re: pgsql: Make WAL segment size configurable at initdb time.

[David Steele]

Hi Andres,

On 10/5/18 5:54 PM, Andres Freund wrote:

On 2018-09-20 11:48:08 -0400, David Steele wrote:


diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e1073ac6d3..3bfd172441 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8440,10 +8440,8 @@ dynamic_library_path = 
'C:\tools\postgresql;H:\my_project\lib;$libdir'


 
-Reports the number of blocks (pages) in a WAL segment file.
-The total size of a WAL segment file in bytes is equal to
-wal_segment_size multiplied by 
wal_block_size;
-by default this is 16MB.  See  for
+Reports the size of write ahead log segments.
+The default value is 16MB. See  for
  more information.
 



Why is this actually more correct? You mean because we have a conversion
that does the mb conversion at display time?


In pre-11 versions of Postgres, you get this:

postgres=# select setting, unit from pg_settings where name = 
'wal_segment_size';

 setting | unit
-+--
 2048| 8kB

But in v11 you get this:

select setting, unit from pg_settings where name = 'wal_segment_size';
 setting  | unit
--+--
 16777216 | B

So, while the WAL segment size used to be expressed in terms of 8K pages 
it is now expressed in terms of absolute bytes.  This seemed to me to be 
a very deliberate change in the original commit so I guessed it was done 
for clarity, but that the docs didn't get the message.


Regards,
--
-David
da...@pgmasters.net

Re: pgsql: Make WAL segment size configurable at initdb time.

[David Steele]

On 9/21/18 12:44 PM, David Steele wrote:

On 9/21/18 12:53 AM, Michael Paquier wrote:

On Thu, Sep 20, 2018 at 06:23:54PM -0700, Andres Freund wrote:

16*M*B, right?  If so, that's normal - pg_settings just reports the
values in the underlying unit - which is XLOG_BLCKSZ, compile-time
defaulting to 8KB. 8192 * 2048 = 16MB.  That's the same in various other
settings.


Would it bring less confusion if we append something like "When querying
pg_settings"?  I can see David's point the current phrasing is
confusing: we don't know if this comes from pg_settings or from SHOW.
It obviously refers to the former, but one can understand that it refers
to the latter.

A second parameter in config.sgml where this formulation is used is
segment_size.


That's why I used a default of 16MB instead of the byte value, because
segment_size used 1GB instead of the byte value.


Not sure where we are on this, but for the record I still think the 
description in PG11 needs to be corrected as in the patch.  It doesn't 
need to be back-patched and the default seems correct to me.


Thanks,
--
-David
da...@pgmasters.net

Re: pgsql: Make WAL segment size configurable at initdb time.

[David Steele]

Hi Andres,

On 9/20/17 1:04 AM, Andres Freund wrote:
>
> Make WAL segment size configurable at initdb time.

<...>

> https://git.postgresql.org/pg/commitdiff/fc49e24fa69a15efacd5b8958115ed9c43c48f9a
It appears that fc49e24f missed updating the runtime config presets
documentation.

Patch attached.

Regards,
-- 
-David
da...@pgmasters.net
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e1073ac6d3..3bfd172441 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8440,10 +8440,8 @@ dynamic_library_path = 
'C:\tools\postgresql;H:\my_project\lib;$libdir'
   
   

-Reports the number of blocks (pages) in a WAL segment file.
-The total size of a WAL segment file in bytes is equal to
-wal_segment_size multiplied by 
wal_block_size;
-by default this is 16MB.  See  for
+Reports the size of write ahead log segments.
+The default value is 16MB. See  for
 more information.

   

Re: pgsql: Validate page level checksums in base backups

[David Steele]

On 4/3/18 4:48 PM, Michael Banck wrote:


Attached is a patch which does that hopefully:

1. creates two user tables, one large enough for at least 6 blocks
(around 360kb), the other just one block.

2. stops the cluster before scribbling over its data and starts it
afterwards.

3. uses the blocksize (and the pager header size) to determine offsets
for scribbling.


This patch looks reasonable to me.


I've tested it with blocksizes 8 and 32 now, the latter should make sure
that the first table is indeed large enough, but maybe something less
arbitrary than "1 integers" should be used?


It might be quicker to just stop the cluster and then write out an 
arbitrary number of zero pages.  Zero pages are always considered valid 
so you can then corrupt whichever pages you want for testing.


--
-David
da...@pgmasters.net

Re: pgsql: Skip temp tables from basebackup.

[David Steele]

On 3/27/18 10:24 AM, Teodor Sigaev wrote:
>> Hm, seems, Windows doesn't like this patch
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird=2018-03-27
>>
> 
> sorry for wrong URL
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird=2018-03-27%2013%3A30%3A24

Looks like the skip total for Windows is incorrect.  The attached should
fix it.

Thanks,
-- 
-David
da...@pgmasters.net
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl 
b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index e6018de054..32d21ce644 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -183,7 +183,7 @@ unlink "$pgdata/$superlongname";
 # skip on Windows.
 SKIP:
 {
-   skip "symlinks not supported on Windows", 15 if ($windows_os);
+   skip "symlinks not supported on Windows", 17 if ($windows_os);
 
# Move pg_replslot out of $pgdata and create a symlink to it.
$node->stop;

Re: pgsql: Exclude unlogged tables from base backups

[David Steele]

On 3/25/18 3:54 PM, Tom Lane wrote:
> David Steele <da...@pgmasters.net> writes:
>> On 3/25/18 3:22 PM, Tom Lane wrote:
>>> Actually, that code didn't guarantee zero termination under *any*
>>> circumstances; it only happened to work if the stack contained
>>> zeroes to start with.
> 
>> Interesting.  strncpy() says it will pad the destination with NULLs when
>> src is less than the size provided.  Perhaps some compilers don't honor
>> that?
> 
> Yeah, but the "size provided" was the number of characters to be copied
> from the source string, not the size of the destination buffer.  So
> strncpy didn't think it needed to add any nulls.  There's a reason why
> that function is widely disliked --- it's hard to use it in a safe way.

Whoops, how right you are.  I'm generally passing destination buffer
size in these cases and totally misread what this was doing.

-- 
-David
da...@pgmasters.net

Re: pgsql: Exclude unlogged tables from base backups

[David Steele]

On 3/25/18 3:22 PM, Tom Lane wrote:
> David Steele <da...@pgmasters.net> writes:
>> On 3/25/18 2:16 PM, Tom Lane wrote:
>>> Buildfarm member skink (valgrind) has reported this during its last couple
>>> of runs:
> 
>> I think skink is using large values for rel oids and that has exposed a
>> bug.  The strncpy doesn't zero terminate the string if the oid has the
>> max number of characters.  At least, I was able to reproduce under those
>> circumstances.
> 
> Actually, that code didn't guarantee zero termination under *any*
> circumstances; it only happened to work if the stack contained
> zeroes to start with.

Interesting.  strncpy() says it will pad the destination with NULLs when
src is less than the size provided.  Perhaps some compilers don't honor
that?

>> The attached should fix it.
> 
> Found this in my inbox right after pushing a fix.  I did it slightly
> differently, emulating the later rather than earlier calls in reinit.c.
> The earlier ones memset the whole target field because they're concerned
> about being able to hash it, but we don't need that here, just zero
> termination.

Yeah, that's the way I would normally do it, but when I searched
reinit.c the first few hits did memset() so I went with that.

Thanks for taking care of it.

-- 
-David
da...@pgmasters.net

Re: pgsql: Fix interaction of Perl and stdbool.h

[David Steele]

On 3/23/18 11:49 AM, Tom Lane wrote:
> Peter Eisentraut  writes:
>> Fix interaction of Perl and stdbool.h
> 
> Not sure if this broke it or it was already broken, but my compiler
> is now very unhappy.
> 
> In file included from /usr/lib64/perl5/CORE/perl.h:2424,
>  from plperl.h:60,
>  from SPI.xs:18:
> /usr/lib64/perl5/CORE/handy.h:108:1: warning: "bool" redefined

One way to fix this is to mark bool as defined in plperl.c:

#ifndef HAS_BOOL
#  define HAS_BOOL 1
#endif

Regards,
-- 
-David
da...@pgmasters.net

Re: pgsql: Add tests for reinit.c

[David Steele]

On 3/19/18 5:58 PM, Andrew Dunstan wrote:
> On Mon, Mar 19, 2018 at 11:33 PM, David Steele <da...@pgmasters.net> wrote:
>> On 3/19/18 4:15 AM, Andrew Dunstan wrote:
>>>
>>> The attached fixes it. We should probably put the function or something
>>> like it in TestLib.pm, though. The call to pwd is quite tricky - it
>>> needs to be done pretty much exactly like this, not quite sure why, but
>>> direct calls to "pwd -W" via system() or backticks don't work, only this
>>> indirect call works on jacana.
>>
>> Thanks for the patch, Andrew!
>>
>> Peter, would you like me to provide a patch that moves this function to
>> TestLib.pm?  Happy to do it, just don't want patches crossing in the
>> night, as it were.
>>
> 
> I've reworked it and pushed the fix.

Thanks, Andrew!

-- 
-David
da...@pgmasters.net

Re: pgsql: Add tests for reinit.c

[David Steele]

On 3/15/18 11:51 PM, Tom Lane wrote:
> I wrote:
>> Peter Eisentraut  writes:
>>> Add tests for reinit.c
> 
>> BTW, buildfarm member jacana hasn't succeeded at this test once.
>> The failures look like
> 
>> ok 1 - init fork in base exists
>> ok 2 - main fork in base exists
>> error running SQL: 'psql::1: ERROR:  directory 
>> "/home/pgrunner/bf/root/HEAD/pgsql.build/src/test/recovery/tmp_check/tmp_test_wBGG"
>>  does not exist'
>> while running 'psql -XAtq -d port=50531 host=127.0.0.1 dbname='postgres' -f 
>> - -v ON_ERROR_STOP=1' with sql 'CREATE TABLESPACE ts1 LOCATION 
>> '/home/pgrunner/bf/root/HEAD/pgsql.build/src/test/recovery/tmp_check/tmp_test_wBGG''
>>  at 
>> /home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/test/perl//PostgresNode.pm
>>  line 1246.
> 
>> so I hypothesize that there's something wrong with TestLib::tempdir on
>> Windows, but no idea what.
> 
> After further staring at that, I think it's some sort of mingw path
> weirdness.  I notice that the initdb output refers to the PGDATA
> directory as
> 
> c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/src/test/recovery/tmp_check/t_014_unlogged_reinit_main_data/pgdata
> 
> and then after that we see "pg_ctl start" succeeding with just
> 
> /home/pgrunner/bf/root/HEAD/pgsql.build/src/test/recovery/tmp_check/t_014_unlogged_reinit_main_data/pgdata
> 
> but this isn't working:
> 
> /home/pgrunner/bf/root/HEAD/pgsql.build/src/test/recovery/tmp_check/tmp_test_esA3
> 
> perhaps because it lacks the "c:/mingw/msys/1.0" prefix.
> 
> Still no opinion about how to fix it.

I looked around for other examples but the only ones I could find are in
the pg_basebackup tests (010_pg_basebackup.pl), which exclude these
calls for Windows.  I don't have a Windows machine to experiment on.

Michael, any thoughts?

-- 
-David
da...@pgmasters.net