Re: [HACKERS] pg9.6 segfault using simple query (related to use fk for join estimates)

[Simon Riggs]

On 9 May 2016 at 00:24, Tomas Vondra  wrote:

> Hi,
>
> Attached is a minor revision of the patch posted by David a few days ago,
> rebased on the current master (which already includes 68d704 fixing the
> segfault that started this thread).
>
> The modifications are fairly small:
>
> * The 'possibleRef' flag is renamed to 'use_for_estimation' which I think
> better describes the purpose of the flag.
>
> * The mark_useful_foreign_keys now skips foreign keys on a single column,
> as those are not useful for the patch at all. This should further reduce
> performance impact in the common case.
>

Good, thanks.


> * I've slightly reworded some of the comments, hopefully for the better.



> But now the bad news - while reviewing the David's fixup patch, I've
> noticed this comment
>
> /* XXX do we need to add entries for the append_rel_list here? */
>
> and I've realized that the estimation does not quite work with partitioned
> tables, as it only checks foreign keys on the parent tables, but the child
> tables may not use the foreign keys at all, or even use different foreign
> keys (a bit bizzare, but possible).
>
> Obviously the simplest solution would be to simply stop considering
> foreign keys with partitioned tables - that seems a bit unfortunate, but
> AFAIK we don't inspect child tables during planning anyway, and in the
> worst case we fall back to the current estimate.
>
> It might be possible to improve this by checking that all child tables
> have a matching foreign key (referencing the same table), which would allow
> us to handle the case with one side partitioned. And maybe some other (more
> complex) cases, like equi-partitioned tables. But all of this would require
> a fair amount of new code, far more than we should commit in beta mode.
>
>
> To summarize all of this, I think David's patch marking usable foreign
> keys greatly reduces the overhead compared to the committed version. The
> 'skip 1-column FKs' significantly reduces (or even eliminates) the overhead
> for the common case where only few FKs use multiple columns.
>
> Not handling the inheritance properly is clearly a serious oversight,
> though. Tom is clearly right that this got committed a bit too early.
>
> If the conclusion is that the current performance impact is still not
> acceptable, or that we need a better solution to the inheritance issue than
> simply disabling the FK estimates, then I think reverting the patch is the
> only possible solution at this point.


I disagree.

The purpose of the patch is to improve planning in simple and important
common cases. It does that. The fact that it does not cover all cases is
understood and we will make more progress in future releases. We don't
handle the inheritance case well in many ways is not this patches' fault,
or problem.

There's no point putting years of effort into parallel query if we can't
work out when to use it sensibly. This patch makes major gains in that area.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Reviewing freeze map code

[Masahiko Sawada]

On Tue, May 3, 2016 at 6:48 AM, Andres Freund  wrote:
> fd31cd2 Don't vacuum all-frozen pages.

-   appendStringInfo(, _("pages: %u removed,
%u remain, %u skipped due to pins\n"),
+   appendStringInfo(, _("pages: %u removed,
%u remain, %u skipped due to pins, %u skipped frozen\n"),

vacrelstats->pages_removed,
 vacrelstats->rel_pages,
-
vacrelstats->pinskipped_pages);
+
vacrelstats->pinskipped_pages,
+
vacrelstats->frozenskipped_pages);

The verbose information about skipping frozen page is emitted by only
autovacuum.
But I think that this information is also helpful for manual vacuum.

Please find attached patch which fixes that.

Regards,

--
Masahiko Sawada
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 426e756..fa6e5fa 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -1316,6 +1316,8 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
 	"Skipped %u pages due to buffer pins.\n",
 	vacrelstats->pinskipped_pages),
 	 vacrelstats->pinskipped_pages);
+	appendStringInfo(, _("Skipped %u frozen pages.\n"),
+	 vacrelstats->frozenskipped_pages);
 	appendStringInfo(, ngettext("%u page is entirely empty.\n",
 	"%u pages are entirely empty.\n",
 	empty_pages),

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Add TAP tests for pg_dump

[Michael Paquier]

On Sun, May 8, 2016 at 6:44 AM, Stephen Frost  wrote:
> I do think that now is a good time for people to be reviewing what's
> been committed, which includes writing tests (either automated ones,
> which can be included in our test suite, or one-off's for testing
> specific things but which don't make sense to include).

And what if the tests are buggy? New test suites should go through a
CF first I think for proper review. And this is clearly one.

> Regarding when we should stop adding new tests, I can't agree with the
> notion that they should be treated as new features.  New tests may break
> the buildfarm, but they do not impact the stability of committed code,
> nor do they introduce new bugs into the code which has been committed
> (if anything, they expose committed bugs, as the set of tests we're
> discussing did, which should then be fixed).

Yes, new tests do not impact the core code, but they could hide
existing bugs, which is what I think Peter is arguing about here, and
why it is not a good idea to pushed in a complete new test suite just
before a beta release. The buildfarm is made so as a run stops once of
the tests turns red, not after all of them have run.

> As such, I'd certainly encourage you to propose new tests, even now, but
> not changes to the PG code, except for bug fixes, or changes agreed to
> by the RMT.  How you prioritise that with the release preparation work
> is up to you, of course, though if I were in your shoes, I'd take care
> of the release prep first, as we're quite close to doing a set of
> releases.  I'm happy to try and help with that effort myself, though
> I've not been involved very much in release prep and am not entirely
> sure what I can do to help.

Adding new tests that are part of an existing bug fix is fine because
the failure of such a test would mean that the bug fix is not working
as intended. Based on my reading of the code this adds test coverage
that is larger than the basic test for a bug fix.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] parallel.c is not marked as test covered

[David Rowley]

On 9 May 2016 at 14:26, Alvaro Herrera  wrote:
> David Rowley wrote:
>> On 9 May 2016 at 13:20, Alvaro Herrera  wrote:
>
>> > It's not a buildfarm machine, but a machine setup specifically for
>> > coverage reports.  It runs "make check-world" only.  I can add some
>> > additional command(s) to run, if somebody can suggest something.
>>
>> I'm not familiar with what's possible with make check-world, but would
>> it be reasonable to make that just do another regression test run with
>> force_parallel_mode set to regress?
>
> My suggestion isn't to change what "make check-world" does; it's just to
> change the script in the coverage machine so that it runs some other
> command after "make check-world" and before "make coverage-html".

I understand what you meant. I was just taking the suggestion one step
further as I thought that if you were adding that for the coverage
test then it might actually also be a good idea that it occurred in
make check-world. The parallel tests are quite thin as it is, so
perhaps its a good idea to get more machines running through the
parallel code.

pg_regress seems to support --temp-config, so we could just have a
config file which has; max_parallel_degree = 2 and force_parallel_mode
= regress.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Stopping logical replication protocol

[Craig Ringer]

I've created a CF entry for this patch:

https://commitfest.postgresql.org/10/621/

set as waiting-on-author. Vladimir, I didn't find a PostgreSQL community
user account for you, so I couldn't set you up as the author. What's your
PostgreSQL community username?

Re: [HACKERS] Reviewing freeze map code

[Masahiko Sawada]

On Sun, May 8, 2016 at 3:18 PM, Masahiko Sawada  wrote:
> On Sat, May 7, 2016 at 11:08 PM, Masahiko Sawada  
> wrote:
>> On Sat, May 7, 2016 at 6:00 AM, Joshua D. Drake  
>> wrote:
>>> On 05/06/2016 01:58 PM, Stephen Frost wrote:

 * Joshua D. Drake (j...@commandprompt.com) wrote:
>
> Yeah I thought about that, it is the word "FORCE" that bothers me.
> When you use FORCE there is an assumption that no matter what, it
> plows through (think rm -f). So if we don't use FROZEN, that's cool
> but FORCE doesn't work either.


 Isn't that exactly what this FORCE option being contemplated would do
 though?  Plow through the entire relation, regardless of what the VM
 says is all frozen or not?

 Seems like FORCE is a good word for that to me.
>>>
>>>
>>> Except that we aren't FORCING a vacuum. That is the part I have contention
>>> with. To me, FORCE means:
>>>
>>> No matter what else is happening, we are vacuuming this relation (think
>>> locks).
>>>
>>> But I am also not going to dig in my heals. If that is truly what -hackers
>>> come up with, thank you at least considering what I said.
>>>
>>> Sincerely,
>>>
>>> JD
>>>
>>
>> As Joshua mentioned, FORCE word might imply doing VACUUM while plowing
>> through locks.
>> I guess that it might confuse the users.
>> IMO, since this option will be a way for emergency, SCANALL word works for 
>> me.
>>
>> Or other ideas are,
>> VACUUM IGNOREVM
>> VACUUM RESCURE
>>
>
> Oops, VACUUM RESCUE is correct.
>

Attached draft patch adds SCANALL option to VACUUM in order to scan
all pages forcibly while ignoring visibility map information.
The option name is SCANALL for now but we could change it after got consensus.

Regards,

--
Masahiko Sawada
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 19fd748..130a70e 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -21,9 +21,9 @@ PostgreSQL documentation
 
  
 
-VACUUM [ ( { FULL | FREEZE | VERBOSE | ANALYZE } [, ...] ) ] [ table_name [ (column_name [, ...] ) ] ]
-VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ table_name ]
-VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] ANALYZE [ table_name [ (column_name [, ...] ) ] ]
+VACUUM [ ( { FULL | FREEZE | VERBOSE | ANALYZE | SCANALL } [, ...] ) ] [ table_name [ (column_name [, ...] ) ] ]
+VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ SCANALL ] [ table_name ]
+VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ SCANALL ] ANALYZE [ table_name [ (column_name [, ...] ) ] ]
 
  
 
@@ -120,6 +120,17 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] ANALYZE [ 

 

+SCANALL
+
+ 
+  Selects forcibly full page scanning vacuum while ignoring visibility map.
+  Forcibly full page scanning vacuum is always performed when the table is
+  rewritten so this option is redundant when FULL is specified.
+ 
+
+   
+   
+   
 ANALYZE
 
  
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 426e756..85e04ac 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -138,7 +138,7 @@ static BufferAccessStrategy vac_strategy;
 
 /* non-export function prototypes */
 static void lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
-			   Relation *Irel, int nindexes, bool aggressive);
+			   Relation *Irel, int nindexes, bool aggressive, bool scan_all);
 static void lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats);
 static bool lazy_check_needs_freeze(Buffer buf, bool *hastup);
 static void lazy_vacuum_index(Relation indrel,
@@ -185,6 +185,7 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params,
 	double		read_rate,
 write_rate;
 	bool		aggressive;		/* should we scan all unfrozen pages? */
+	bool		scan_all;		/* should we scan all pages forcibly? */
 	bool		scanned_all_unfrozen;	/* actually scanned all such pages? */
 	TransactionId xidFullScanLimit;
 	MultiXactId mxactFullScanLimit;
@@ -233,6 +234,9 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params,
 	aggressive |= MultiXactIdPrecedesOrEquals(onerel->rd_rel->relminmxid,
 			  mxactFullScanLimit);
 
+	/* If SCANALL option is specified, we have to scan all pages forcibly */
+	scan_all = options & VACOPT_SCANALL;
+
 	vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats));
 
 	vacrelstats->old_rel_pages = onerel->rd_rel->relpages;
@@ -246,14 +250,14 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params,
 	vacrelstats->hasindex = (nindexes > 0);
 
 	/* Do the vacuuming */
-	lazy_scan_heap(onerel, vacrelstats, Irel, nindexes, aggressive);
+	lazy_scan_heap(onerel, vacrelstats, Irel, nindexes, aggressive, scan_all);
 
 	/* Done with indexes */
 	vac_close_indexes(nindexes, Irel, NoLock);
 
 	/*
-	 * Compute whether we actually scanned the whole relation. If we did, we
-	 * can adjust relfrozenxid and relminmxid.
+	 * Compute whether 

Re: [HACKERS] Stopping logical replication protocol

[Craig Ringer]

On 6 May 2016 at 23:23, Vladimir Gordiychuk  wrote:


> I prepare small patch that fix problems describe below. Now *WalSndWriteData
> *first check message from consumer and during decode transaction check
> that replication still active.
>
OK, upon looking closer I'm not sure I agree with this approach.

In WalSndLoop(...) we currently call ProcessRepliesIfAny() which handles a
client-initiated CopyDone by replying with its own CopyDone. WalSndLoop
then just waits until the send buffer is flushed or the client disconnects
and returns. That looks to be pretty much what's needed... but only when we
actually execute that code path.

XLogSendLogical calls WalSndWaitForWal(...) (via xlogreader and
logical_read_xlog_page) when waiting for something to process, when the
client will be blocked on a socket read. It also processes client CopyDone,
but it doesn't seem to test streamingDoneSending or streamingDoneReceiving
like the outer WalSndLoop does. So it looks like it continues waiting for
WAL when we know the client sent CopyDone and doesn't want anything more.

I think we should be testing that in WalSndWaitForWal, like we do in
WalSndLoop, and bailing out. You've done that part, and I agree that's good.

It seems like what you're also trying to allow interruption deeper than
that, when we're in the middle of processing a reorder buffer commit record
and streaming that to the client. You're introducing an is_active member
(actually a callback, though name suggests it's a flag) in struct
ReorderBuffer to check whether a CopyDone is received, and you're skipping
ReorderBuffer commit processing when the callback returns false. The
callback returns "!streamingDoneReceiving && !streamingDoneSending" i.e.
it's false if either end has sent CopyDone. streamingDoneSending and
streamingDoneSending are only set in ProcessRepliesIfAny, called by
WalSndLoop and WalSndWaitForWal. So the idea is, presumably, that if we're
waiting for WAL from XLogSendLogical we skip processing of any commit
records and exit.

That seems overcomplicated.

When WalSndWaitForWAL is called
by logical_read_xlog_page, logical_read_xlog_page can just test
streamingDoneReceiving and streamingDoneSending. If they're set it can skip
the page read and return -1, which will cause the xlogreader to return a
null record to XLogSendLogical. That'll skip the decoding calls and return
to WalSndLoop, where we'll notice it's time to exit.

That way there's no need to modify the reorder buffer structs, add
callbacks, etc.

Make sense?

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] parallel.c is not marked as test covered

[Alvaro Herrera]

David Rowley wrote:
> On 9 May 2016 at 13:20, Alvaro Herrera  wrote:

> > It's not a buildfarm machine, but a machine setup specifically for
> > coverage reports.  It runs "make check-world" only.  I can add some
> > additional command(s) to run, if somebody can suggest something.
> 
> I'm not familiar with what's possible with make check-world, but would
> it be reasonable to make that just do another regression test run with
> force_parallel_mode set to regress?

My suggestion isn't to change what "make check-world" does; it's just to
change the script in the coverage machine so that it runs some other
command after "make check-world" and before "make coverage-html".

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] First-draft release notes for next week's back-branch releases

[David G. Johnston]

On Sunday, May 8, 2016, Tom Lane  wrote:

> [ I think you meant to attach this to the other thread, but anyway... ]


This is where the link to the online version was; reading the sgml
and/or compiling ends up being a bit more than I wanted to do to review
these.


>
> "David G. Johnston" > writes:
> > "...replacement_sort_tuples, which see for further details." needs
> > rewording.
>
> Hmm, "which see" is perfectly good English to my knowledge, and I'm not
> sure that other possible ways of wording this would be less awkward.
>

Removing it doesn't seem like a bad choice...The user should realize the
relevant preceding linked guc is where they should look for more details -
pointing it out to them seems verbose.  But the meaning is clear regardless
of familiarity.


> > Is it worth mentioning the deprecation of exclusive backups in the notes
> > introducing non-exclusive ones?
>
> It's not clear to me that we're actually deprecating them; there did not
> seem to be consensus on that.


>
Then section 24.3.3 needs fixing. The second paragraph explicitly states it
is deprecated.

http://www.postgresql.org/docs/devel/static/continuous-archiving.html

David J.

Re: [HACKERS] parallel.c is not marked as test covered

[David Rowley]

On 9 May 2016 at 13:20, Alvaro Herrera  wrote:
> David Rowley wrote:
>
>> I'm not entirely sure which machine generates that coverage output,
>> but the problem with it is that it's just one machine. We do have at
>> least one buildfarm member which runs with force_parallel_mode =
>> regress.
>
> It's not a buildfarm machine, but a machine setup specifically for
> coverage reports.  It runs "make check-world" only.  I can add some
> additional command(s) to run, if somebody can suggest something.

I'm not familiar with what's possible with make check-world, but would
it be reasonable to make that just do another regression test run with
force_parallel_mode set to regress?

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Stopping logical replication protocol

[Craig Ringer]

On 6 May 2016 at 23:23, Vladimir Gordiychuk  wrote:


> I prepare small patch that fix problems describe below. Now *WalSndWriteData
> *first check message from consumer and during decode transaction check
> that replication still active. KeppAlive message now not send if was get
> CopyDone package(keep alive now not necessary we preparing to complete). 
> *WalSndWaitForWal
> *after get CopyDone exit from loop. With apply this patch I get next
> measurements
>

I'll review this, but based on the description and concept I agree it's
useful.

I have been frustrated in the past by the inability to terminate the
logical replication stream and return to command mode without a
disconnect/reconnect so I'd like to have this.

AFAIK there's no particular reason we can't do it, it's just not
implemented. It does have to be a clean exit though, where the logical
decoding context is destroyed, the xact it was running in is closed, etc.
Like for the SQL interface.

You still won't want to do it too often because there's a cost to stopping
decoding and restarting it. We have to re-read from the restart_lsn and
reassemble transactions again up to the point where we can start sending
them to the client again. Especially if you're most of the way through
decoding a big xact, or if there's a long-running xact holding restart_lsn
down this might cost a bit. But no worse than repeatedly calling the SQL
logical decoding interface.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] parallel.c is not marked as test covered

[Alvaro Herrera]

David Rowley wrote:

> I'm not entirely sure which machine generates that coverage output,
> but the problem with it is that it's just one machine. We do have at
> least one buildfarm member which runs with force_parallel_mode =
> regress.

It's not a buildfarm machine, but a machine setup specifically for
coverage reports.  It runs "make check-world" only.  I can add some
additional command(s) to run, if somebody can suggest something.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] parallel.c is not marked as test covered

[David Rowley]

On 9 May 2016 at 09:12, Clément Prévost  wrote:
> The entire parallel.c reported test coverage is zero:
> http://coverage.postgresql.org/src/backend/access/transam/parallel.c.gcov.html
>
> It seem that it's not covered by the original 924bcf4f commit but I don't
> know if it's on purpose. This feature being really new that would be
> understandable.
>
> If it's not, my first idea would be to fix this by adding a simple sql test
> in /src/test/regress, in the "sql" and "expected" subdirectories that
> explain (cost off) some queries.
> I'm also guessing here that we can force a query to have a parallel plan
> with some cost magic to avoid dealing with sufficiently large datasets.

I'm not entirely sure which machine generates that coverage output,
but the problem with it is that it's just one machine. We do have at
least one buildfarm member which runs with force_parallel_mode =
regress. See 
http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=mandrill=HEAD

If the coverage was run from that machine then you'd see something
higher than 0% on parallel.c.

It would be nice to improve this a bit and have the planner generate a
handful of parallel plans even without force_parallel_mode = regress.
I do believe we can now do this without having to create large tables
in the regression tests if we set parallel_setup_cost to something
low, perhaps 0 and set the table's parallel_degree to something higher
than 1. The problem with that is knowing what should actually be
tested. It seems like pretty much any plan which can generate a
parallel query is a good candidate for writing a test for, which makes
the choices for which tests to write quite hard. It makes you realise
why Robert when down the force_parallel_mode = regress path instead.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] A population of population counts

[Thomas Munro]

On Sat, May 7, 2016 at 4:26 PM, Tom Lane  wrote:
> David Rowley  writes:
>> I'd like to see us using those functions, when they're available and
>> falling back on the array when they're not. Sounds like that would
>> just be a new configure test. Perhaps a good home for some shared code
>> would be numutils.c.
>
> Meh --- numutils.c is about numbers.  Maybe "bitutils.c"?
>
> Another point here is that there might easily be a use-case for such
> functionality in frontend code, so I'd lean towards putting the support in
> src/common if we do this.

I played around with this a bit on the weekend (see rough sketch
attached).  The trouble with __builtin_popcount and the POPCNT
instruction is that you only get them if you ask for -msse4.2 or
-mpopcnt, and then you get an illegal instruction trap instead of
sensible fallback behaviour on ancient hardware, so no packager would
be able to do that.  So I guess we'd have to use a runtime test like
we do for CRC32 hardware support (the test may actually be identical
since both depend on the SSE4.2 instruction set) and maybe inline
assembler rather the GCC builtin, and that seems a bit over the top
unless someone can show that it's worth it.

My aim with this thread was mainly reducing code duplication and
needless code: perhaps at least the other ideas in the attached
sketch, namely using ffs instead of the rightmost_one_pos table loop
and consolidation of popcount into a reusable API (without trying to
get hardware support) could be worth polishing for the next CF?
Annoyingly, it seems Windows doesn't have POSIX/SUSv2 ffs, though
apparently it can reach that instruction with MSVC intrinsic
_BitScanReverse or MingW __builtin_ffs.

-- 
Thomas Munro
http://www.enterprisedb.com


popcount-and-rightmostbitpos-table-cleanup.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] First-draft release notes for next week's back-branch releases

[Gavin Flower]

On 09/05/16 10:22, Tom Lane wrote:

Gavin Flower  writes:

On 09/05/16 08:56, Tom Lane wrote:

Hmm, "which see" is perfectly good English to my knowledge, and I'm not
sure that other possible ways of wording this would be less awkward.

To me the phrase "which see" is plain weird, at least in this context!
Is this some American usage I've not heard on TV nor films???

Don't think so.  AFAIK it's a translation of the Latin "q.v." (quod vide),
and is used in more or less the same way.  It's not hard to find examples
by googling.

regards, tom lane


Well I've come across many examples of examples of bad grammar, so 
finding an example of  usage in Google is not proof the usage is valid!


Even at best, it doesn't flow and is awkward.


Cheers,
Gavin




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg9.6 segfault using simple query (related to use fk for join estimates)

[Tomas Vondra]

Hi,

Attached is a minor revision of the patch posted by David a few days 
ago, rebased on the current master (which already includes 68d704 fixing 
the segfault that started this thread).


The modifications are fairly small:

* The 'possibleRef' flag is renamed to 'use_for_estimation' which I 
think better describes the purpose of the flag.


* The mark_useful_foreign_keys now skips foreign keys on a single 
column, as those are not useful for the patch at all. This should 
further reduce performance impact in the common case.


* I've slightly reworded some of the comments, hopefully for the better.


But now the bad news - while reviewing the David's fixup patch, I've 
noticed this comment


/* XXX do we need to add entries for the append_rel_list here? */

and I've realized that the estimation does not quite work with 
partitioned tables, as it only checks foreign keys on the parent tables, 
but the child tables may not use the foreign keys at all, or even use 
different foreign keys (a bit bizzare, but possible).


Obviously the simplest solution would be to simply stop considering 
foreign keys with partitioned tables - that seems a bit unfortunate, but 
AFAIK we don't inspect child tables during planning anyway, and in the 
worst case we fall back to the current estimate.


It might be possible to improve this by checking that all child tables 
have a matching foreign key (referencing the same table), which would 
allow us to handle the case with one side partitioned. And maybe some 
other (more complex) cases, like equi-partitioned tables. But all of 
this would require a fair amount of new code, far more than we should 
commit in beta mode.



To summarize all of this, I think David's patch marking usable foreign 
keys greatly reduces the overhead compared to the committed version. The 
'skip 1-column FKs' significantly reduces (or even eliminates) the 
overhead for the common case where only few FKs use multiple columns.


Not handling the inheritance properly is clearly a serious oversight, 
though. Tom is clearly right that this got committed a bit too early.



If the conclusion is that the current performance impact is still not 
acceptable, or that we need a better solution to the inheritance issue 
than simply disabling the FK estimates, then I think reverting the patch 
is the only possible solution at this point.



regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


fkest_fixes_v2.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] First-draft release notes for next week's back-branch releases

[Tom Lane]

Gavin Flower  writes:
> On 09/05/16 08:56, Tom Lane wrote:
>> Hmm, "which see" is perfectly good English to my knowledge, and I'm not
>> sure that other possible ways of wording this would be less awkward.

> To me the phrase "which see" is plain weird, at least in this context!  
> Is this some American usage I've not heard on TV nor films???

Don't think so.  AFAIK it's a translation of the Latin "q.v." (quod vide),
and is used in more or less the same way.  It's not hard to find examples
by googling.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] First-draft release notes for next week's back-branch releases

[Gavin Flower]

On 09/05/16 08:56, Tom Lane wrote:

[ I think you meant to attach this to the other thread, but anyway... ]

"David G. Johnston"  writes:

"...replacement_sort_tuples, which see for further details." needs
rewording.

Hmm, "which see" is perfectly good English to my knowledge, and I'm not
sure that other possible ways of wording this would be less awkward.


[...]

To me the phrase "which see" is plain weird, at least in this context!  
Is this some American usage I've not heard on TV nor films???


English is my first language, I was born in England and now reside in 
New Zealand.



Cheers,
Gavin


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] parallel.c is not marked as test covered

[Clément Prévost]

The entire parallel.c reported test coverage is zero:
http://coverage.postgresql.org/src/backend/access/transam/parallel.c.gcov.html

It seem that it's not covered by the original 924bcf4f commit but I don't
know if it's on purpose. This feature being really new that would be
understandable.

If it's not, my first idea would be to fix this by adding a simple sql test
in /src/test/regress, in the "sql" and "expected" subdirectories that
explain (cost off) some queries.
I'm also guessing here that we can force a query to have a parallel plan
with some cost magic to avoid dealing with sufficiently large datasets.

Re: [HACKERS] First-draft release notes for next week's back-branch releases

[Tom Lane]

[ I think you meant to attach this to the other thread, but anyway... ]

"David G. Johnston"  writes:
> "...replacement_sort_tuples, which see for further details." needs
> rewording.

Hmm, "which see" is perfectly good English to my knowledge, and I'm not
sure that other possible ways of wording this would be less awkward.

> Is it worth mentioning the deprecation of exclusive backups in the notes
> introducing non-exclusive ones?

It's not clear to me that we're actually deprecating them; there did not
seem to be consensus on that.

I adopted your other suggestions.  Thanks for reviewing!

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Greg Stark]

For what it's worth, for my historical sort benchmarks I got Postgres
to build right back to 6.5 using modern tools. I have patches if
anyone wants them. Pre-7.3 doesn't actually run because we didn't
support 64-bit architectures before Tom did the V1 api (there was a
set of Alpha patches floating around but they don't seem sufficient
for x86_64). But I suspect if I built it for x86 32-bit that would
clear the immediate problem.


I was considering proposing backpatching some minimal patches to get
old unsupported branches to at least build with modern tools and run.
Just to make it easy for people to test historical behaviour and I
suppose pg_dump or other client testing would be a good use case too.
I was also going to propose turning off all warnings on these
unsupported back branches while I was at it.


One other lesson I learned doing this was that it was a pain referring
to individual git checkouts because we don't have a tag for point
where we branched the new development. So all my git-describes were
ending up with things like REL9_2_BETA2-619-gff6c78c or
REL9_3_BETA1-925-g6668ad1. You just had to know that REL9_3_BETA1-xxx
was probably a revision sometime during PG 9.5 development since BETA1
was probably where we forked development whereas 9.3 probably forked
off 9_2_BETA2. (And some were things like REL7_4_RC1-1513-g4525418
instead)

I would suggest adding tags for each version on the first development
revision so that we could see things like REL9_5_DEV-nnn would mean
the nnnth commit on the 9.5 development branch.

(What I actually did instead myself was use git describe --tags
--contains $i --match 'REL[0-9]_[0-9]_[0-9]' which gave me things like
REL9_5_0~1334 which means the 1334th commit *before* REL9_5_0. That
was also confusing but at least consistent)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] force_parallel_mode uniqueness

[David G. Johnston]

On Sun, May 8, 2016 at 10:56 AM, Robert Haas  wrote:

> On Sat, May 7, 2016 at 11:42 PM, David G. Johnston
>  wrote:
> > All of the other planner GUCs are basically, {on, off, special} with on
> or
> > special the default as appropriate for the feature - since most/all
> features
> > default to enabled.  While I get that the expected usage is to set this
> to
> > off (which really leaves parallel mode in its default on behavior) and
> then
> > reduce the parallel workers to zero to disable that runs contrary to all
> of
> > the other switches listed alongside force_parallel_mode.
> > constraint_exclusion seems like something to be emulated here.
>
> I am not really sure what you are suggesting here.  If you're saying
> that you don't like the ordering regress > on > off, because there are
> other GUCs where the intermediate values are all between "on" and
> "off", then I think that's silly.  We should name and order the
> options based on what makes sense, not based on what made sense for
> other options.  Note that if you think there are no other GUCs which
> have a value greater than "on", see also
> synchronous_commit=remote_apply.
>

​I was thinking more along the lines that it should be called:

parallel_mode (enum)

It would default to "on"

"off" would turn it off (instead of having to set parallel_degree to 0)

And it would have additional enum values for:

"always" - basically what on means in the current setup
"regress" - the same as the current regress.​


> > Also, all of the other geoq options get placed here yet max parallel
> degree
> > is in an entirely different section.
>
> max_parallel_degree has nothing to do with GEQO, so I don't know that
> the location of "other" GEQO options has much to do with anything.  It
> also has nothing to do with force_parallel_mode, which is what this
> email was about until you abruptly switched topics.
>

​I was simply trying to indicate that the various settings that configure
geqo are present on this page.  max_parallel_degree is likewise a setting
that configures parallel_mode but it isn't on this page.​


> > I'm a bit torn on this point though
> > since it does fit nicely in asynchronous behavior.  I think the next
> thought
> > finds the happy middle.
>
> We could put max_parallel_degree under "other planner options" rather
> than "asynchronous behavior".  However, I wonder what behavior people
> will want for parallel operations that are not queries.  For example,
> suppose we have parallel CREATE INDEX.  Should the number of workers
> for that operation also be controlled by max_parallel_degree?  If yes,
> then this shouldn't be a query planner option, because CREATE INDEX is
> not a query.
>

​Like I said, it isn't clear-cut.  But at the moment it is just for queries
- it could be moved if it gets dual purposed as you describe.


> > If nothing else this option should include a link to max_parallel_degree
> and
> > vice-versa.  Noting how to disable the feature in this section, if the
> guc
> > semantics are not changed, would be good too.  That note would likely
> > suffice to establish the linking term to parallel degree.  Something can
> be
> > devised, even if just a see also, to link back.
>
> It's probably worth mentioning under force_parallel_mode that it will
> have no effect if parallel query is disabled by the
> max_parallel_degree setting.  But it is completely unnecessary IMHO
> for max_parallel_degree to link to force_parallel_mode.  Most people
> should not be using force_parallel_mode; it is there for testing
> whether functions are correctly labeled as to parallel safety and
> that's it.
>

So this particular capability is unique and as such it warrants offing a
"force" mode that none of the other planner configuration GUCs on this page
have.  Its clear that this is how it was intended but as a casual reader of
the section its uniqueness stood out - and maybe that is for the better.

I guess part of the misunderstanding is simply that you have a lot more
plans for this feature than are currently implemented but I am reading the
documentation only knowing about those parts that are.

David J.

Re: [HACKERS] force_parallel_mode uniqueness

[Robert Haas]

On Sat, May 7, 2016 at 11:42 PM, David G. Johnston
 wrote:
> All of the other planner GUCs are basically, {on, off, special} with on or
> special the default as appropriate for the feature - since most/all features
> default to enabled.  While I get that the expected usage is to set this to
> off (which really leaves parallel mode in its default on behavior) and then
> reduce the parallel workers to zero to disable that runs contrary to all of
> the other switches listed alongside force_parallel_mode.
> constraint_exclusion seems like something to be emulated here.

I am not really sure what you are suggesting here.  If you're saying
that you don't like the ordering regress > on > off, because there are
other GUCs where the intermediate values are all between "on" and
"off", then I think that's silly.  We should name and order the
options based on what makes sense, not based on what made sense for
other options.  Note that if you think there are no other GUCs which
have a value greater than "on", see also
synchronous_commit=remote_apply.

> Also, all of the other geoq options get placed here yet max parallel degree
> is in an entirely different section.

max_parallel_degree has nothing to do with GEQO, so I don't know that
the location of "other" GEQO options has much to do with anything.  It
also has nothing to do with force_parallel_mode, which is what this
email was about until you abruptly switched topics.

> I'm a bit torn on this point though
> since it does fit nicely in asynchronous behavior.  I think the next thought
> finds the happy middle.

We could put max_parallel_degree under "other planner options" rather
than "asynchronous behavior".  However, I wonder what behavior people
will want for parallel operations that are not queries.  For example,
suppose we have parallel CREATE INDEX.  Should the number of workers
for that operation also be controlled by max_parallel_degree?  If yes,
then this shouldn't be a query planner option, because CREATE INDEX is
not a query.

> If nothing else this option should include a link to max_parallel_degree and
> vice-versa.  Noting how to disable the feature in this section, if the guc
> semantics are not changed, would be good too.  That note would likely
> suffice to establish the linking term to parallel degree.  Something can be
> devised, even if just a see also, to link back.

It's probably worth mentioning under force_parallel_mode that it will
have no effect if parallel query is disabled by the
max_parallel_degree setting.  But it is completely unnecessary IMHO
for max_parallel_degree to link to force_parallel_mode.  Most people
should not be using force_parallel_mode; it is there for testing
whether functions are correctly labeled as to parallel safety and
that's it.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] minor message improvement

[Simon Riggs]

On 8 May 2016 at 12:48, Simon Riggs  wrote:

> On 8 May 2016 at 03:53, Euler Taveira  wrote:
>
> Attached is a patch that turn it into one.
>>
>
> Applied, by Tom. Thanks
>

Sorry about that; Tom's message only just arrived with me for some reason.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [COMMITTERS] pgsql: Disable BLOB test in pg_dump TAP tests

[Stephen Frost]

* Andrew Dunstan (and...@dunslane.net) wrote:
> TL;DR version: Msys provides a virtualized Unix-like file system
> that is visible to its programs that you use to build, but invisible
> to the programs you build since they are going to run without any
> knowledge of the build environment.  So you can't just put $tmpdir
> from an Msys-aware program into a psql script and expect it to work.
> It won't. Maybe we need to provide a function in our test library to
> convert virtual paths to real paths. (For Unix it would be a NOOP).

I'm pretty confident that I can work around this for the specific test
that was having trouble, but it seems like it'd be nice to have a way to
refer to the actual data and temp directories from psql and friends when
they're run under the TAP test system.

Ideally, there'd be a way to make it transparent, so we don't have to
write the tests to handle this case explicitly, though I'm not sure if
there's an easy way to do that.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Re: "pg_xxx" role name restriction not applied to bootstrap superuser?

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> If we're going to enforce such a restriction, I think it would be
> >> a good thing for it to be in place in beta1.
> 
> > Makes sense.
> > Patch attached.  I'll push this in a bit, barring objections.
> 
> Three minor wording quibbles:
> 
> * s/reserved/disallowed/ maybe?  Not 100% sold on this.
> 
> * s/can not/cannot/
> 
> * use double quotes not single around pg_

Pushed those changes and also emailed the buildfarm owner directly
regarding the change.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Re: "pg_xxx" role name restriction not applied to bootstrap superuser?

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> If we're going to enforce such a restriction, I think it would be
> >> a good thing for it to be in place in beta1.
> 
> > Makes sense.
> > Patch attached.  I'll push this in a bit, barring objections.
> 
> Three minor wording quibbles:
> 
> * s/reserved/disallowed/ maybe?  Not 100% sold on this.
> 
> * s/can not/cannot/
> 
> * use double quotes not single around pg_

Blargh.  Missed this before pushing, sorry.

Will fix.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] Re: new tests post-feature freeze (was pgsql: Add TAP tests for pg_dump)

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> Also, if we say that new tests are not features, that would mean that
> they could be back-patched even after the release is out the door, and
> generally I'm not in favor of that policy, except when we're adding a
> test to a back-branch that is closely tied to a bug we are fixing in
> that branch.  I do not want to see the pg_dump test suite back-patched
> to all active branches, for example, even though I approve of having
> test coverage for pg_dump.  I am confident that minimizing churn in
> the back-branches is a more important goal than ensuring test coverage
> for those branches, and that we will regret it if we reverse those
> priorities.

I agree that it doesn't make sense to back-patch large test suites which
are primairly intended to provide code-coverage testing.  In many cases,
that would simply be duplicative without much gain as the code hasn't
changed and tests would have to be removed due to features which don't
exist in older branches, and there is certainly a risk there that it
could complicate things for organizations which run the test suites and
for packagers unnecessairly.

> My suggestion is that, from this point forward, we add new tests to
> 9.6 only if they are closely related to a bug that is getting fixed or
> a feature that is new in 9.6.  I think that's a reasonable compromise,
> but what do others think?

I'm willing to accept that compromise, but I'm not thrilled with it due
to what it will mean for the process I'm currently going through.  The
approach I've been using has been to add tests to gain more code
coverage of the code in pg_dump.  That has turned up multiple
pre-existing bugs in pg_dump but the vast majority of the tests come
back clean.  This compromise would mean that I'd continue to work
through the code coverage tests, but would have to segregate out and
commit only those tests which actually reveal bugs, once those bugs have
been fixed (as to avoid turning the buildfarm red).  The rest of the
tests would still get written, but since they currently don't reveal
bugs, they would be shelved until development is opened for 9.7.

Thinking further on this, I'd probably end up creating a buildfarm
animal which only reports to me which runs the full set of tests, so
that any regression which occurs that the tests catch during the beta
period is caught.

Unfortunately, I'm not able to only work on and write tests only for
bugs, as I don't know what the bugs are.  Writing new tests using the
test suite isn't really any more work than doing one-off testing, but
it has lasting value that one-off testing doesn't.  I don't mean to say
"I accept the compromise but will do my own thing anyway" but rather to
point out that this is an efficient and worthwhile way to find bugs and
that's what I'm planning to work on to help ensure that we're ready to
release.

I'm happy to look at reviewing other committed patches also, but I've
had my head in pg_dump for the past few months and have a pretty good
handle on it, for the moment anyway, and there have certainly been lots
of complaints over the years about our lack of test coverage for it.
Were I to look at reviewing and testing other patches, well, at the
moment I'd be pretty inclined to write test cases for those too, as it
seems to be working to find issues.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] new tests post-feature freeze (was pgsql: Add TAP tests for pg_dump)

[Tom Lane]

Robert Haas  writes:
> My suggestion is that, from this point forward, we add new tests to
> 9.6 only if they are closely related to a bug that is getting fixed or
> a feature that is new in 9.6.  I think that's a reasonable compromise,
> but what do others think?

Yeah, that's fair.  I suspect that Peter's unhappiness with these tests
is partly rooted in the fact that they're just generic pg_dump tests
and not connected to any new-in-9.6 behavior.  As such, it's not entirely
clear why we should be taking any stability risk by pushing them in
so late.  We seem to have gotten away with it this time --- but it would
only have taken one more problem to get me to switch my vote to "revert
them".

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: "pg_xxx" role name restriction not applied to bootstrap superuser?

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> If we're going to enforce such a restriction, I think it would be
>> a good thing for it to be in place in beta1.

> Makes sense.
> Patch attached.  I'll push this in a bit, barring objections.

Three minor wording quibbles:

* s/reserved/disallowed/ maybe?  Not 100% sold on this.

* s/can not/cannot/

* use double quotes not single around pg_

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] new tests post-feature freeze (was pgsql: Add TAP tests for pg_dump)

[Robert Haas]

On Sat, May 7, 2016 at 5:44 PM, Stephen Frost  wrote:
> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> On 5/7/16 9:36 AM, Stephen Frost wrote:
>> >Honestly, over the next couple of months between feature-freeze and
>> >release, I'd like to add even more tests, and not just to pg_dump but
>> >also to other commands that don't have very good testing today (psql, in
>> >particular, but pg_dumpall needs more also, and there's more to do with
>> >pg_dump too).
>>
>> If we're going to do that, there will be no stopping it.  I also
>> have a bunch of code in this area lined up, but I was hoping to
>> submit it to the commit fest process at the right time and order to
>> get feedback and testing.  If we're going to allow new tests being
>> developed right up until release, I can just stop doing release
>> preparation work right now and get back to coding and reviewing.
>
> I do think that now is a good time for people to be reviewing what's
> been committed, which includes writing tests (either automated ones,
> which can be included in our test suite, or one-off's for testing
> specific things but which don't make sense to include).
>
> That doesn't mean we should be codeing or reviewing new features for
> commit.
>
> As for release prep, I certainly applaud everyone involved in that
> effort as we do have the beta release and back-branch releases coming
> up.
>
> Regarding when we should stop adding new tests, I can't agree with the
> notion that they should be treated as new features.  New tests may break
> the buildfarm, but they do not impact the stability of committed code,
> nor do they introduce new bugs into the code which has been committed
> (if anything, they expose committed bugs, as the set of tests we're
> discussing did, which should then be fixed).
>
>> Ultimately, tests are code and should be treated as such.
>
> Perhaps in some manners this is accurate, but I'd view our test suite as
> an independent code base from PG.  Bugs in the test suite might cause
> false test failures or other issues on the buildfarm or during
> packaging, but bugs or issues in the test suite won't cause data loss,
> data corruption, or generally impact running operations of our users.
>
> I can see some sense in holding off on adding more tests once we hit RC,
> as we want anything done with RC to be essentially identical to the
> release, unless there is a serious issue, but holding off on adding new
> tests which could expose issues in committed code for the months during
> beta doesn't seem sensible.
>
> As such, I'd certainly encourage you to propose new tests, even now, but
> not changes to the PG code, except for bug fixes, or changes agreed to
> by the RMT.  How you prioritise that with the release preparation work
> is up to you, of course, though if I were in your shoes, I'd take care
> of the release prep first, as we're quite close to doing a set of
> releases.  I'm happy to try and help with that effort myself, though
> I've not been involved very much in release prep and am not entirely
> sure what I can do to help.

In the end, the question of the degree to which tests constitute
features is one that needs to be made by the whole community, not
individual developers.  I think you both raise good points. On the one
hand, developing new test frameworks could distract from other release
preparation tasks, as Peter rightly points out.  On the other hand, it
could also make the release more reliable, as Stephen points out.  I
believe that there is a clear consensus that at least some new tests
are welcome even post-feature freeze. However, I also believe that we
could get carried away with that, and end up having it become a
distraction from the task of getting the release out the door.

Also, if we say that new tests are not features, that would mean that
they could be back-patched even after the release is out the door, and
generally I'm not in favor of that policy, except when we're adding a
test to a back-branch that is closely tied to a bug we are fixing in
that branch.  I do not want to see the pg_dump test suite back-patched
to all active branches, for example, even though I approve of having
test coverage for pg_dump.  I am confident that minimizing churn in
the back-branches is a more important goal than ensuring test coverage
for those branches, and that we will regret it if we reverse those
priorities.

My suggestion is that, from this point forward, we add new tests to
9.6 only if they are closely related to a bug that is getting fixed or
a feature that is new in 9.6.  I think that's a reasonable compromise,
but what do others think?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] pgsql: Disable BLOB test in pg_dump TAP tests

[Andrew Dunstan]

On 05/08/2016 12:52 AM, Stephen Frost wrote:

Tom,

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

Stephen Frost  writes:

Disable BLOB test in pg_dump TAP tests
Buildfarm member jacana appears to have an issue with running this
test.  It's not entirely clear to me why, but rather than try to
fight with it, just disable it for now.

BTW, what was your evidence for thinking that that specific test
needed to be disabled?  It looks to me like it would have been
subject to the $-in-/m-mode bug we identified today, so I'm wondering
if it was merely the first to fail.

No, those errors were "match/don't match" errors.  At the bottom of:

http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=jacana=2016-05-06%2023%3A41%3A15=bin-check

is the error, which is 'No such file or directory'.  I'm pretty sure the
error is from the '\o' in that test which is trying to write a file to
the '$tempdir' directory.  Clearly, on that system at least, $tempdir
isn't a completely fully-qualified path, since it ends up being:

/home/pgrunner/bf/root/HEAD/pgsql.build/src/bin/pg_dump/tmp_check/tmp_test_v8cH

but the data directory is:

c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/src/bin/pg_dump/tmp_check/data_main_NSG0/pgdata

I'm not quite sure how that system works (is a starting '/' actually
referring to a relative path, and psql isn't being run from that root
directory?  Or does the shell handle converting arguments which start
with a '/' to their full path, and that's why calls to pg_ctl and other
things work, but trying to use that same path from inside a program
doesn't?), but apparently referring to $tempdir from inside psql doesn't
give you the same directory that referring to it from the TAP perl
script does.




TL;DR version: Msys provides a virtualized Unix-like file system that is 
visible to its programs that you use to build, but invisible to the 
programs you build since they are going to run without any knowledge of 
the build environment.  So you can't just put $tmpdir from an Msys-aware 
program into a psql script and expect it to work. It won't. Maybe we 
need to provide a function in our test library to convert virtual paths 
to real paths. (For Unix it would be a NOOP).


cheers

andrew





--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: "pg_xxx" role name restriction not applied to bootstrap superuser?

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> ... but I'm left with a policy question: should initdb disallow
> >> bootstrap superuser names like "pg_xxx"?
> 
> > On the whole, I'd vote to treat the bootstrap user as a normal role and
> > therefore have the same restriction in place for that user also.
> 
> If we're going to enforce such a restriction, I think it would be
> a good thing for it to be in place in beta1.

Makes sense.

Patch attached.  I'll push this in a bit, barring objections.

Thanks!

Stephen
From ae3ec5c409464612754cd36372a0fc2166bc2f62 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Sun, 8 May 2016 08:35:16 -0400
Subject: [PATCH] Disallow superuser names starting with 'pg_' in initdb

As with CREATE ROLE, disallow users from specifying initial
superuser names which begin with 'pg_' in initdb.

Per discussion with Tom.
---
 src/bin/initdb/initdb.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 299ddfe..7dedd8a 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -3562,6 +3562,12 @@ main(int argc, char *argv[])
 	if (strlen(username) == 0)
 		username = effective_user;
 
+	if (strncmp(username, "pg_", 3) == 0)
+	{
+		fprintf(stderr, _("%s: superuser name \"%s\" is reserved; role names can not begin with 'pg_'\n"), progname, username);
+		exit(1);
+	}
+
 	printf(_("The files belonging to this database system will be owned "
 			 "by user \"%s\".\n"
 			 "This user must also own the server process.\n\n"),
-- 
2.5.0



signature.asc
Description: Digital signature

Re: [HACKERS] minor message improvement

[Simon Riggs]

On 8 May 2016 at 03:53, Euler Taveira  wrote:

Attached is a patch that turn it into one.
>

Applied, by Tom. Thanks

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Reviewing freeze map code

[Masahiko Sawada]

On Sat, May 7, 2016 at 11:08 PM, Masahiko Sawada  wrote:
> On Sat, May 7, 2016 at 6:00 AM, Joshua D. Drake  
> wrote:
>> On 05/06/2016 01:58 PM, Stephen Frost wrote:
>>>
>>> * Joshua D. Drake (j...@commandprompt.com) wrote:

 Yeah I thought about that, it is the word "FORCE" that bothers me.
 When you use FORCE there is an assumption that no matter what, it
 plows through (think rm -f). So if we don't use FROZEN, that's cool
 but FORCE doesn't work either.
>>>
>>>
>>> Isn't that exactly what this FORCE option being contemplated would do
>>> though?  Plow through the entire relation, regardless of what the VM
>>> says is all frozen or not?
>>>
>>> Seems like FORCE is a good word for that to me.
>>
>>
>> Except that we aren't FORCING a vacuum. That is the part I have contention
>> with. To me, FORCE means:
>>
>> No matter what else is happening, we are vacuuming this relation (think
>> locks).
>>
>> But I am also not going to dig in my heals. If that is truly what -hackers
>> come up with, thank you at least considering what I said.
>>
>> Sincerely,
>>
>> JD
>>
>
> As Joshua mentioned, FORCE word might imply doing VACUUM while plowing
> through locks.
> I guess that it might confuse the users.
> IMO, since this option will be a way for emergency, SCANALL word works for me.
>
> Or other ideas are,
> VACUUM IGNOREVM
> VACUUM RESCURE
>

Oops, VACUUM RESCUE is correct.

Regards,

--
Masahiko Sawada


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers