date:20180227

Re: Server won't start with fallback setting by initdb.

2018-02-27 Thread Michael Paquier

On Wed, Feb 14, 2018 at 10:08:06AM +0900, Kyotaro HORIGUCHI wrote:
> Definitely. The another rationale for the value is that regtest
> fails with the numbers less than 20. So it's not 11 but
> 20. Currently regtest should succeed with that number of
> connections as written in parallel_schedule and I've read (in
> Robert's mail, but I haven't confirmed) that tests for parallel
> scans are designed to use up to 20 connections.

I just noticed, but this thread in registered in next CF, so I have
switched this patch as ready for committer.  In the documentation,
guc-max-connections (config.sgml) mentions that the default value of
max_connections is typically 100, but it could be less as determined by
initdb.  Do we want to specify as well that initdb would use 100 as
upper bound and 20 as lower bound when it does its evaluation?  I would
think that this is not worth mentioning in the docs but as initdb is
directly mentioned..
--
Michael


signature.asc
Description: PGP signature

Re: PATCH: Configurable file mode mask

2018-02-27 Thread Michael Paquier

On Tue, Feb 27, 2018 at 03:52:32PM -0500, David Steele wrote:
> On 1/30/18 3:01 AM, Michael Paquier wrote:
> The purpose of this test to be ensure nothing else is in the directory.
> As the tests get more complex I think keeping track of the state will be
> important.  In other words, this is really an assertion that the current
> test state is correct, not that the prior command succeeded.

Good point.  Objection removed.

>> Another test that could be easily included is with -o, then create a
>> table and check for its OID value.  Also for -x, then just use
>> txid_current to check the value consistency.  For -c, with
>> track_commit_timestamp set to on, you can set a couple of values and
>> then check for pg_last_committed_xact() which would be NULL if you use
>> an XID older than the minimum set for example.  All those are simple
>> enough so they could easily be added in the first version of this test
>> suite.
> 
> I would prefer to leave this for another patch.

OK, no problems with that either.

>>> 2) 02-mkdir
>>>
>>> Converts mkdir() calls to MakeDirectoryDefaultPerm() if the original
>>> call used default permissions.
>> 
>> So the only call not converted to that API is in ipc.c...  OK, this one
>> is pretty simple so nothing really to say about it, the real meat comes
>> with patch 3.  I would rather see with a good eye if this patch
>> introduces file_perm.h which includes both PG_FILE_MODE_DEFAULT and
>> PG_DIR_MODE_DEFAULT, and have the frontends use those values.  This
>> would make the switch to 0003 a bit easier if you look at pg_resetwal's
>> diffs for example.
> 
> Agreed.  I thought about this originally but could not come up with a
> good split.  I spent some time on it and came up with something that I
> think works (and would make sense to commit separately).

OK, thanks.

>> +command_ok(
>> +   ['chmod', "-R", 'g+rX', "$pgdata"],
>> +   'add group perms to PGDATA');
>>
>> That would blow up on Windows.  You should replace that by perl's chmod
>> and look at its return result for sanity checks, likely with ($cnt != 0).
>> And let's not use icacls please...
> 
> Fixed with a new function, add_pg_data_group_perm().

That's basically a recursive chmod, so chmod_recursive is more adapted?
I could imagine that this is useful as well for removing group
permissions, so the new mode could be specified as an argument.

>> +if ($params{has_group_access})
>> +{
>> +push(@{$params{extra}}, '-g');
>> +}
>> No need to introduce a new parameter here, please use directly extra =>
>> [ '-g' ] in the routines calling PostgresNode::init.
> 
> Other code needs to know if group access is enabled on the node (see
> RewindTest.pm) so it's not just a matter of passing the option.

I don't quite understand here.  I have no objection into extending
setup_cluster() with a group_access argument so as the tests run both
the group access and without it, but I'd rather keep the low-level API
clean of anything fancy if we can use the facility which already
exists.

> New patches are attached.  The end result is almost identical to v6 but
> code was moved from 03 to 02 as per Michael's suggestion.

Thanks for the updated versions.

> 1) 01-pgresetwal-test
> 
> Adds a very basic test suite for pg_resetwal.

Okay.  This one looks fine to me.  It could be passed down to a
committer.

> 2) 02-file-perm
> 
> Adds a new header (file_perm.h) and makes all front-end utilities use
> the new constants for setting umask.  Converts mkdir() calls in the
> backend to MakeDirectoryDefaultPerm() if the original
> call used default permissions.  Adds tests to make sure permissions in
> PGDATA are set correctly by the front-end tools.

I had a more simple (complicated?) thing in mind for the split, which
would actually cause this patch to be split into two more:
1) Introduce file_perm.h and replace all the existing values for modes
and masks to what the header introduces.  This allows to check if the
new header is not forgotten anywhere, especially for the frontends.
2) Introduce MakeDirectoryDefaultPerm, which switches the backend calls
of mkdir() to the new API.
3) Add the grouping facilities, which updates the default masks and
modes of file_perm.h.

Grouping 1) and 2) together as you do makes sense as well I guess, as in
my proposal the mkdir calls in the backend would be touched twice, still
things get more incremental.

+/*
+ * Default mode for created files.
+ */
+#define PG_FILE_MODE_DEFAULT   (S_IRUSR | S_IWUSR | S_IRGRP)
+
+/*
+ * Default mode for directories.
+ */
+#define PG_DIR_MODE_DEFAULT(S_IRWXU | S_IRGRP | S_IXGRP)
For the first cut, this should not use S_IRGRP in the first declaration,
and (S_IXGRP | S_IXGRP) in the second declaration.

-if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+if (script == NULL && (script = fopen(output_path, "w")) == NULL)
Why are those calls in pg_upgrade changed?

TestLib.pm does not need the group-related extensions, r

Re: [HACKERS] Restrict concurrent update/delete with UPDATE of partition key

2018-02-27 Thread Rajkumar Raghuwanshi

On Wed, Feb 14, 2018 at 5:44 PM, Amit Kapila 
wrote:

> +# Concurrency error from GetTupleForTrigger
> +# Concurrency error from ExecLockRows
>
> I think you don't need to mention above sentences in spec files.
> Apart from that, your patch looks good to me.  I have marked it as
> Ready For Committer.
>

I too have tested this feature with isolation framework and this look good
to me.

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

RE: [bug fix] pg_rewind takes long time because it mistakenly copies data files

2018-02-27 Thread Tsunakawa, Takayuki

From: Michael Paquier [mailto:mich...@paquier.xyz]
> So I would propose to just do that later.  I have looked a second time at
> your patch, attached is the set of tests I have run:

Thanks so much, that has helped me a lot!

> I have one small comment though.  The comment block at the beginning of
> isRelDataFile() refers to "pg_tblspc//PG_9.4_201403261/".
> Could you switch that to "pg_tblspc//"?
> That's not directly the fault of your patch, but as long as we are on it..

Done, thanks again for marking the CF entry.

Regards
Takayuki Tsunakawa



pg_rewind_pathcmp_v2.patch
Description: pg_rewind_pathcmp_v2.patch

Re: [bug fix] pg_rewind takes long time because it mistakenly copies data files

2018-02-27 Thread Michael Paquier

On Mon, Feb 26, 2018 at 06:24:02PM +0900, Michael Paquier wrote:
> Anything like that would be work only for HEAD I think as that's a bit
> of refactoring. And indeed it could give you a good introduction to the
> TAP facility.

So I would propose to just do that later. I have looked a second time
at your patch, attached is the set of tests I have run:
- With assertions enabled, I see pg_rewind failing on an assertion as
mentioned upthread.
- With assertion disabled (look at rewind_logs.log as generated by the
script attached), then all the files from the tablespace are copied with
all the file chunks fetched at the second phase of the rewind.
- With the patch, both the assertion and the handling of tablespace
files are fixed. Again, in the logs produced by the script you would
see that tablespace files are not completely copied anymore, and that
only portions of them are.

I have one small comment though. The comment block at the beginning of
isRelDataFile() refers to "pg_tblspc//PG_9.4_201403261/".
Could you switch that to "pg_tblspc//"?
That's not directly the fault of your patch, but as long as we are on
it..

So I am marking this as ready for committer. Thanks for the report,
Tsunakawa-san!
--
Michael
#!/bin/bash

set -e

PRIMARY_PGDATA=$HOME/data/primary
STANDBY_PGDATA=$HOME/data/standby
PRIMARY_TBSPACE=$HOME/data/tbspace
STANDBY_TBSPACE=$HOME/data/tbspace_standby

rm -rf $PRIMARY_PGDATA $STANDBY_PGDATA
rm -rf $PRIMARY_TBSPACE
rm -rf $STANDBY_TBSPACE

mkdir $PRIMARY_TBSPACE

# Create primary with its tablespace
initdb --noclean --nosync -D $PRIMARY_PGDATA
cat >> $PRIMARY_PGDATA/postgresql.conf <> $STANDBY_PGDATA/postgresql.conf < $STANDBY_PGDATA/recovery.conf < $HOME/data/rewind_logs.log
echo "port=5432" >> $PRIMARY_PGDATA/postgresql.conf
cat > $PRIMARY_PGDATA/recovery.conf <

1 2 >

1 - 100 of 104 matches

Mail list logo