Re: [HACKERS] Test code is worth the space

[Noah Misch]

On Fri, Aug 14, 2015 at 12:47:49PM +0100, Simon Riggs wrote:
> On 13 August 2015 at 00:31, Robert Haas  wrote:
> > On Wed, Aug 12, 2015 at 7:20 PM, Tom Lane  wrote:
> > > We've talked about having some sort of second rank of tests that
> > > people wouldn't necessarily run before committing, and that would
> > > be allowed to eat more time than the core regression tests would.
> > > I think that might be a valuable direction to pursue if people start
> > > submitting very bulky tests.
> >
> > Maybe.  Adding a whole new test suite is significantly more
> > administratively complex, because the BF client has to get updated to
> > run it.  And if expected outputs in that test suite change very often
> > at all, then committers will have to run it before committing anyway.
> >
> > The value of a core regression suite that takes less time to run has
> > to be weighed against the possibility that a better core regression
> > suite might cause us to find more bugs before committing.  That could
> > easily be worth the price in runtime.
> 
> Seems like a simple fix. We maintain all regression tests in full, but keep
> slow tests in separate files accessed only by a different schedule.
> 
> make check == fast-parallel_schedule
> make check-full == parallel_schedule

+1 for a split, though I would do "make quickcheck" and "make check".  Using
fewer tests should be a conscious decision, and "check" is the widely-known
Makefile target.  In particular, non-hackers building production binaries need
the thorough test battery.  (As a bonus, the buildfarm wouldn't miss a beat.)


-- 
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] Test code is worth the space

[Noah Misch]

On Wed, Aug 12, 2015 at 06:46:19PM +0100, Greg Stark wrote:
> On Wed, Aug 12, 2015 at 3:10 AM, Noah Misch  wrote:
> > Committers press authors to delete tests more often than we press them to
> > resubmit with more tests.  No wonder so many patches have insufficient 
> > tests;
> > we treat those patches more favorably, on average.  I have no objective
> > principles for determining whether a test is pointlessly redundant, but I
> > think the principles should become roughly 10x more permissive than the
> > (unspecified) ones we've been using.
> 
> I would suggest the metric should be "if this test fails is it more
> likely to be noise due to an intentional change in behaviour or more
> likely to be a bug?"

When I've just spent awhile implementing a behavior change, the test diffs are
a comforting sight.  They confirm that the test suite exercises the topic I
just changed.  Furthermore, most tests today do not qualify under this
stringent metric you suggest.  The nature of pg_regress opposes it.

I sometimes hear a myth that tests catch the bugs their authors anticipated.
We have tests like that (opr_sanity comes to mind), but much test-induced bug
discovery is serendipitous.  To give a recent example, Peter Eisentraut didn't
write src/bin tests to reveal the bug that led to commit d73d14c.


-- 
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] Raising our compiler requirements for 9.6

[Noah Misch]

On Sun, Aug 16, 2015 at 05:58:17AM +0200, Andres Freund wrote:
> On 2015-08-15 23:50:09 -0400, Noah Misch wrote:
> > On Sun, Aug 16, 2015 at 02:03:01AM +0200, Andres Freund wrote:
> > > On 2015-08-15 12:47:09 -0400, Noah Misch wrote:
> > > > $ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1'
> > > > pg_resetxlog.o: In function `fastgetattr':
> > > > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754:
> > > >  undefined reference to `nocachegetattr'
> > > > pg_resetxlog.o: In function `heap_getattr':
> > > > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783:
> > > >  undefined reference to `heap_getsysattr'
> > 
> > > What's the advantage of using STATIC_IF_INLINE over just #ifndef
> > > FRONTEND?
> > 
> > > In fact STATIC_IF_INLINE does *not* even help here unless we also detect
> > > compilers that support inlining but balk when inline functions reference
> > > symbols not available. There was an example of that in the buildfarm as
> > > of 2014, c.f. a9baeb361d63 . We'd have to disable inlining for such
> > > compilers.

> > Solaris Studio 12.3 (newest version as of Oct 2014) still does that
> > when optimization is disabled, and I place sufficient value on keeping
> > inlining enabled for such a new compiler.
> 
> Ah, that's cool. I was wondering generally how we could find an animal
> to detect that case once pademelon met its untimely (or timely by now?)
> end.

I would just make a "gcc -O0 -DPG_FORCE_DISABLE_INLINE=1" animal, since the
Solaris machine time supply is small.

> > The policy would then be
> > (already is?) to wrap in "#ifdef FRONTEND" any inline function that uses a
> > backend symbol.  When a header is dedicated to such functions, we might 
> > avoid
> > the whole header in the frontend instead of wrapping each function.  That
> > policy works for me.
> 
> Cool. I was wondering before where we'd document policy around
> this. sources.sgml?

+1.  Most coding rules go undocumented, but I'm in favor of changing that as
the opportunity arises.


-- 
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] Raising our compiler requirements for 9.6

[Andres Freund]

On 2015-08-15 23:50:09 -0400, Noah Misch wrote:
> On Sun, Aug 16, 2015 at 02:03:01AM +0200, Andres Freund wrote:
> > On 2015-08-15 12:47:09 -0400, Noah Misch wrote:
> > > $ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1'
> > > pg_resetxlog.o: In function `fastgetattr':
> > > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754:
> > >  undefined reference to `nocachegetattr'
> > > pg_resetxlog.o: In function `heap_getattr':
> > > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783:
> > >  undefined reference to `heap_getsysattr'
> 
> > What's the advantage of using STATIC_IF_INLINE over just #ifndef
> > FRONTEND?
> 
> > In fact STATIC_IF_INLINE does *not* even help here unless we also detect
> > compilers that support inlining but balk when inline functions reference
> > symbols not available. There was an example of that in the buildfarm as
> > of 2014, c.f. a9baeb361d63 . We'd have to disable inlining for such
> > compilers.
> 
> Neat; I didn't know that.

Personally I don't find that particularly neat, rather annoying actually
:P

> Solaris Studio 12.3 (newest version as of Oct 2014) still does that
> when optimization is disabled, and I place sufficient value on keeping
> inlining enabled for such a new compiler.

Ah, that's cool. I was wondering generally how we could find an animal
to detect that case once pademelon met its untimely (or timely by now?)
end.

> The policy would then be
> (already is?) to wrap in "#ifdef FRONTEND" any inline function that uses a
> backend symbol.  When a header is dedicated to such functions, we might avoid
> the whole header in the frontend instead of wrapping each function.  That
> policy works for me.

Cool. I was wondering before where we'd document policy around
this. sources.sgml?

Greetings,

Andres Freund


-- 
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] Raising our compiler requirements for 9.6

[Noah Misch]

On Sun, Aug 16, 2015 at 02:03:01AM +0200, Andres Freund wrote:
> On 2015-08-15 12:47:09 -0400, Noah Misch wrote:
> > $ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1'
> > pg_resetxlog.o: In function `fastgetattr':
> > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754:
> >  undefined reference to `nocachegetattr'
> > pg_resetxlog.o: In function `heap_getattr':
> > /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783:
> >  undefined reference to `heap_getsysattr'

> What's the advantage of using STATIC_IF_INLINE over just #ifndef
> FRONTEND?

> In fact STATIC_IF_INLINE does *not* even help here unless we also detect
> compilers that support inlining but balk when inline functions reference
> symbols not available. There was an example of that in the buildfarm as
> of 2014, c.f. a9baeb361d63 . We'd have to disable inlining for such
> compilers.

Neat; I didn't know that.  Solaris Studio 12.3 (newest version as of Oct 2014)
still does that when optimization is disabled, and I place sufficient value on
keeping inlining enabled for such a new compiler.  The policy would then be
(already is?) to wrap in "#ifdef FRONTEND" any inline function that uses a
backend symbol.  When a header is dedicated to such functions, we might avoid
the whole header in the frontend instead of wrapping each function.  That
policy works for me.

On Sat, Aug 15, 2015 at 01:48:17PM -0400, Tom Lane wrote:
> Realistically, with what we're doing now, we *have* broken things for
> stupid-about-inlines compilers; because even if everything in the core
> distribution builds, we've almost certainly created failures for
> third-party modules that need to #include headers that no contrib
> module includes.

Only FRONTEND code (e.g. repmgr, pg_reorg) will be at hazard, not ordinary
third-party (backend) modules.  We could have a test frontend that includes
every header minus a blacklist, but I don't think it's essential.  External
FRONTEND code is an order of magnitude less common than external backend code.
Unlike backend module development, we don't document the existence of the
FRONTEND programming environment.


-- 
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] Autonomous Transaction is back

[Noah Misch]

On Sat, Aug 15, 2015 at 10:20:55PM -0300, Alvaro Herrera wrote:
> Noah Misch wrote:
> 
> > In today's scenarios, the later query cannot commit unless the suspended 
> > query
> > also commits.  (Changing that is the raison d'être of autonomous
> > transactions.)  If the autonomous transaction can interact with uncommitted
> > work in a way that other backends could not, crazy things happen when the
> > autonomous transaction commits and the suspended transaction aborts:
> > 
> > CREATE TABLE t (c) AS SELECT 1;
> > BEGIN;
> > UPDATE t SET c = 2 WHERE c = 1;
> > BEGIN_AUTONOMOUS;
> > UPDATE t SET c = 3 WHERE c = 1;
> > UPDATE t SET c = 4 WHERE c = 2;
> > COMMIT_AUTONOMOUS;
> > ROLLBACK;
> > 
> > If you replace the autonomous transaction with a savepoint, the c=3 update
> > finds no rows, and the c=4 update changes one row.  When the outer 
> > transaction
> > aborts, only the original c=1 row remains live.  If you replace the 
> > autonomous
> > transaction with a dblink/pg_background call, the c=3 update waits
> > indefinitely for c=2 to commit or abort, an undetected deadlock.
> 
> Maybe what we need to solve this is to restrict what the autonomous
> transaction can do; for instance, make it so that the autonomous
> transaction can see all rows of the outer transaction as if the outer
> transaction were committed, but trying to update any such row raises an
> error.  As far as I can see, this closes this particular problem.  (We
> likely need additional rules to close all holes, but hopefully you get
> the idea.)
> 
> Perhaps there exists a set of rules strong enough to eliminate all
> problematic visibility scenarios, but which still enables behavior
> useful enough to cover the proposed use cases.  The audit scenario is
> covered because the audit trail doesn't need to modify the audited
> tuples themselves, only read them.

My starting expectation is that the semantics of an autonomous transaction
will be exactly those of dblink/pg_background.  (I said that during the
unconference session.)  The application would need to read data from tables
before switching to the autonomous section.  Autonomous transactions are then
a performance and syntactic help, not a source of new semantics.  Does any
database have autonomous transactions that do otherwise?


-- 
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] Autonomous Transaction is back

[Alvaro Herrera]

Noah Misch wrote:

> In today's scenarios, the later query cannot commit unless the suspended query
> also commits.  (Changing that is the raison d'être of autonomous
> transactions.)  If the autonomous transaction can interact with uncommitted
> work in a way that other backends could not, crazy things happen when the
> autonomous transaction commits and the suspended transaction aborts:
> 
> CREATE TABLE t (c) AS SELECT 1;
> BEGIN;
> UPDATE t SET c = 2 WHERE c = 1;
> BEGIN_AUTONOMOUS;
> UPDATE t SET c = 3 WHERE c = 1;
> UPDATE t SET c = 4 WHERE c = 2;
> COMMIT_AUTONOMOUS;
> ROLLBACK;
> 
> If you replace the autonomous transaction with a savepoint, the c=3 update
> finds no rows, and the c=4 update changes one row.  When the outer transaction
> aborts, only the original c=1 row remains live.  If you replace the autonomous
> transaction with a dblink/pg_background call, the c=3 update waits
> indefinitely for c=2 to commit or abort, an undetected deadlock.

Maybe what we need to solve this is to restrict what the autonomous
transaction can do; for instance, make it so that the autonomous
transaction can see all rows of the outer transaction as if the outer
transaction were committed, but trying to update any such row raises an
error.  As far as I can see, this closes this particular problem.  (We
likely need additional rules to close all holes, but hopefully you get
the idea.)

Perhaps there exists a set of rules strong enough to eliminate all
problematic visibility scenarios, but which still enables behavior
useful enough to cover the proposed use cases.  The audit scenario is
covered because the audit trail doesn't need to modify the audited
tuples themselves, only read them.

-- 
Á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] Raising our compiler requirements for 9.6

[Andres Freund]

On 2015-08-15 12:47:09 -0400, Noah Misch wrote:
> Atomics were a miner's canary for pademelon's trouble with post-de6fd1c
> inlining.  Expect pademelon to break whenever a frontend-included file gains
> an inline function that calls a backend function.  Atomics were the initial
> examples, but this patch adds more:

That's a good point.

> $ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1'
> pg_resetxlog.o: In function `fastgetattr':
> /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754:
>  undefined reference to `nocachegetattr'
> pg_resetxlog.o: In function `heap_getattr':
> /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783:
>  undefined reference to `heap_getsysattr'
> 
> The htup_details.h case is trickier in a way, because pg_resetxlog has a
> legitimate need for SizeofHeapTupleHeader via TOAST_MAX_CHUNK_SIZE.  Expect
> this class of problem to recur as we add more inline functions.  Our method to
> handle these first two instances will set a precedent.
> 
> That gave me new respect for STATIC_IF_INLINE.  While it does add tedious work
> to the task of introducing a new batch of inline functions, the work is
> completely mechanical.  Anyone can write it; anyone can review it; there's one
> correct way to write it.

What's the advantage of using STATIC_IF_INLINE over just #ifndef
FRONTEND? That doesn't work well for entire headers in my opinion, but
for individual functions it shouldn't be a problem? In fact we've done
it for years for MemoryContextSwitchTo(). And by the problem definition
such functions cannot be required by frontend code.

STATIC_IF_INLINE is really tedious because to know whether it works you
essentially need to recompile postgres with inlines enabled/disabled.

In fact STATIC_IF_INLINE does *not* even help here unless we also detect
compilers that support inlining but balk when inline functions reference
symbols not available. There was an example of that in the buildfarm as
of 2014, c.f. a9baeb361d63 . We'd have to disable inlining for such
compilers.

> Header surgery like
> 0001-Don-t-include-low-level-locking-code-from-frontend-c.patch
> requires expert design from scratch, and it (trivially) breaks builds
> for a sample of non-core code.

I agree that such surgery isn't always a good idea. In my opinion the
removing proc.h from the number of headers in the above and the followon
WIP patch I posted has value completely independently from fixing
problems.

Greetings,

Andres Freund


-- 
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] CREATE POLICY and RETURNING

[Zhaomo Yang]

Stephen,

If no NEW or OLD is used, what happens?  Or would you have
> to always specify OLD/NEW for UPDATE, and then what about for the other
> policies, and the FOR ALL policies?


I should be clearer with references to OLD/NEW. SELECT Predicates cannot
reference any of them.
INSERT predicates cannot refer to OLD and DELETE predicates cannot refer to
NEW. Basically,
for INSERT/UPDATE/DELETE, we specify predicates the same way as we do for
triggers' WHEN condition.

As for FOR ALL, I think we will abandon it if we apply SELECT policy to
other commands, since SELECT predicate
will be the new universally applicable read policy, which makes the FOR ALL
USING clause much less useful. Of course users may need to specify separate
predicates for different commands, but I think it is fine. How often do
users want the same predicate for all the commands?

This could be accomplished with "USING (bar > 1)" and "WITH CHECK (foo >
> 1)", no?
> Your sentence above that "USING and WITH CHECK are combined by AND"
> isn't correct either- they're independent and are therefore really OR'd.
> If they were AND'd then the new record would have to pass both USING and
> WITH CHECK policies.


No, it is impossible with the current implementation.

CREATE TABLE test {
 id int,
 v1 int,
 v2 int
};

Suppose that the user wants an update policy which is OLD.v1 > 10 OR NEW.v2
< 10.
As you suggested, we use the following policy

CREATE update_p ON test
FOR UPDATE TO test_user
USING v1 > 10
WITH CHECK v2 < 10;

(1) Assume there is only one row in the table
id |  v1 | v2 |
1  | 11 | 20 |

Now we execute  UPDATE test SET v2 = 100.
this query is allowed by the policy and the only row should be updated
since v1's old value > 10, but will trigger an error because it violates
the WITH CHECK clause.

(2) Again assume there is only one row in the table
id |  v1 | v2 |
1  | 9 | 20 |

Now we execute  UPDATE test SET v2 = 7.
this query is allowed by the policy and the only row should be updated
since v2's new value < 10, nothing will be updated because the only row
will be filtered out before update happens.

This is why I said USING and WITH CHECK are combined by AND. In order to
update an row, first the row needs to be visible, which meaning it needs to
pass the USING check, then it needs to pass the WITH CHECK.

Further, I'm not sure that I see how this would work in a case where you
> have the SELECT policy (which clearly could only refer to OLD) applied
> first, as you suggest?


We use SELECT policy to filter the table when we scan it (just like how we
use USING clause now). The predicate of UPDATE will be checked later
(probably similar to how we handle trigger's WHEN clause which can also
reference OLD and NEW).

Thanks,
Zhaomo

Re: [HACKERS] Autonomous Transaction is back

[Noah Misch]

On Fri, Aug 07, 2015 at 11:26:08AM -0400, Robert Haas wrote:
> On Thu, Aug 6, 2015 at 11:04 PM, Merlin Moncure  wrote:
> > I don't necessarily disagree with what you're saying, but it's not
> > clear to me what the proposed behavior is.  Since the AT can commit
> > before the outer, ISTM *any* ungranted lock requested by the AT but
> > held by the outer leads to either A: functional deadlock (regardless
> > of implementation details) or B: special behavior.
> 
> I don't accept that.  We've already GOT cases where a query can be
> suspended and other queries can be running in the same backend.  You
> can do that via cursors.  Those cases work fine, and the deadlock
> detector doesn't know anything about them.  How is this any different?

In today's scenarios, the later query cannot commit unless the suspended query
also commits.  (Changing that is the raison d'être of autonomous
transactions.)  If the autonomous transaction can interact with uncommitted
work in a way that other backends could not, crazy things happen when the
autonomous transaction commits and the suspended transaction aborts:

CREATE TABLE t (c) AS SELECT 1;
BEGIN;
UPDATE t SET c = 2 WHERE c = 1;
BEGIN_AUTONOMOUS;
UPDATE t SET c = 3 WHERE c = 1;
UPDATE t SET c = 4 WHERE c = 2;
COMMIT_AUTONOMOUS;
ROLLBACK;

If you replace the autonomous transaction with a savepoint, the c=3 update
finds no rows, and the c=4 update changes one row.  When the outer transaction
aborts, only the original c=1 row remains live.  If you replace the autonomous
transaction with a dblink/pg_background call, the c=3 update waits
indefinitely for c=2 to commit or abort, an undetected deadlock.

Suppose you make the autonomous transaction see tuples like in the savepoint
case.  The c=3 update finds no rows to update, and the c=4 update changes one
row.  When the outer transaction aborts, you have two live rows (c=1 and c=4).
Suppose you instead make the autonomous transaction see tuples like in the
dblink case, yet let c=3 ignore the lock and change a row.  If both the
autonomous transaction and the outer transaction were to commit, then you get
two live rows (c=2 and c=3).  Neither of those outcomes is acceptable, of
course.  In today's tuple update rules, c=3 must deadlock[1].  Other credible
tuple update rules may not have this problem, but nothing jumps to mind.


[1] That's not to say it must use the shmem lock structures and deadlock
detector.


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

[HACKERS] Potential GIN vacuum bug

[Jeff Janes]

When ginbulkdelete gets called for the first time in a  VACUUM(i.e. stats
== NULL), one of the first things it does is call ginInsertCleanup to get
rid of the pending list.  It does this in lieu of vacuuming the pending
list.

This is important because if there are any dead tids still in the Pending
list, someone else could come along during the vacuum and post the dead
tids into a part of the index that VACUUM has already passed over.

The potential bug is that ginInsertCleanup exits early (ginfast.c lines
796, 860, 898) if it detects that someone else is cleaning up the pending
list, without waiting for that someone else to finish the job.

Isn't this a problem?

Cheers,

Jeff

Re: [HACKERS] Small improvement to get_base_rel_indexes()

[Tom Lane]

David Rowley  writes:
> Attached is a small patch which improves the way get_base_rel_indexes()
> works.

> The current version creates a new bitmapset on each recursion level then
> bms_joins() to the one on the next level up each time. I understand that
> this will patch will have about a 0 net performance improvement, but I
> thought I'd post anyway as:

> 1. It removes 5 lines of code.
> 2. It's a better example to leave in the code.

I don't know that it's a better example: it seems substantially uglier.
(Notably, you complicated the API contract of get_base_rel_indexes()
without documenting the fact: it's now dependent on the original caller
to have initialized *result to NULL.)

We might have to use a method like this if the function were returning
multiple sets; but since it is not, the existing approach seems more
readable and safer to me.

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] Raising our compiler requirements for 9.6

[Tom Lane]

Andres Freund  writes:
> On August 15, 2015 6:47:09 PM GMT+02:00, Noah Misch  wrote:
>> That gave me new respect for STATIC_IF_INLINE.  While it does add
>> tedious work to the task of introducing a new batch of inline
>> functions, the work is completely mechanical.  Anyone can write it;
>> anyone can review it; there's one correct way to write it.  Header
>> surgery like
>> 0001-Don-t-include-low-level-locking-code-from-frontend-c.patch
>> requires expert design from scratch, and it (trivially) breaks builds
>> for a sample of non-core code.  Let's return to STATIC_IF_INLINE and
>> convert fastgetattr() therewith.  (A possible future line of inquiry is
>> to generate the STATIC_IF_INLINE transformation at build time, so the
>> handwritten headers can use C99 inlining directly as though it is
>> always available.)

> I'll respond in detail later. But wouldn't it be easy in this case to
> just ifndef FRONTEND things in this case?

I think that's missing Noah's point.  Yeah, we'll probably always be able
to hack things to continue to work, but the key word there is "hack".
And if we don't have buildfarm coverage for compilers that are stupid
about inlining, we can expect to break the case regularly.  (pademelon
won't last forever, though maybe by the time that box dies we'll figure
we no longer need to care about such compilers.)

I'm not especially in love with STATIC_IF_INLINE, but it did offer a
mechanical if tedious solution.

Realistically, with what we're doing now, we *have* broken things for
stupid-about-inlines compilers; because even if everything in the core
distribution builds, we've almost certainly created failures for
third-party modules that need to #include headers that no contrib
module includes.

Maybe we should just say "okay, we're raising the bar for 9.6: compilers
that don't elide unused static inlines need not apply".  But I'd be
happier if we had a clear idea which those were.  And if we go that route,
we ought to add a configure test checking it.

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] Raising our compiler requirements for 9.6

[Andres Freund]

On August 15, 2015 6:47:09 PM GMT+02:00, Noah Misch  wrote:
>On Tue, Aug 11, 2015 at 01:04:48PM +0200, Andres Freund wrote:
>> On 2015-08-05 15:46:36 +0200, Andres Freund wrote:
>> > Here's a conversion for fastgetattr() and heap_getattr().
>
>> In my opinion this drastically increases readability and thus should
>be
>> applied.
>
>Atomics were a miner's canary for pademelon's trouble with post-de6fd1c
>inlining.  Expect pademelon to break whenever a frontend-included file
>gains
>an inline function that calls a backend function.  Atomics were the
>initial
>examples, but this patch adds more:
>
>$ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1'
>pg_resetxlog.o: In function `fastgetattr':
>/data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754:
>undefined reference to `nocachegetattr'
>pg_resetxlog.o: In function `heap_getattr':
>/data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783:
>undefined reference to `heap_getsysattr'
>
>The htup_details.h case is trickier in a way, because pg_resetxlog has
>a
>legitimate need for SizeofHeapTupleHeader via TOAST_MAX_CHUNK_SIZE. 
>Expect
>this class of problem to recur as we add more inline functions.  Our
>method to
>handle these first two instances will set a precedent.
>
>That gave me new respect for STATIC_IF_INLINE.  While it does add
>tedious work
>to the task of introducing a new batch of inline functions, the work is
>completely mechanical.  Anyone can write it; anyone can review it;
>there's one
>correct way to write it.  Header surgery like
>0001-Don-t-include-low-level-locking-code-from-frontend-c.patch
>requires
>expert design from scratch, and it (trivially) breaks builds for a
>sample of
>non-core code.  Let's return to STATIC_IF_INLINE and convert
>fastgetattr()
>therewith.  (A possible future line of inquiry is to generate the
>STATIC_IF_INLINE transformation at build time, so the handwritten
>headers can
>use C99 inlining directly as though it is always available.)

I'll respond in detail later. But wouldn't it be easy in this case to just 
ifndef FRONTEND things in this case?

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.


-- 
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] Raising our compiler requirements for 9.6

[Noah Misch]

On Tue, Aug 11, 2015 at 01:04:48PM +0200, Andres Freund wrote:
> On 2015-08-05 15:46:36 +0200, Andres Freund wrote:
> > Here's a conversion for fastgetattr() and heap_getattr().

> In my opinion this drastically increases readability and thus should be
> applied.

Atomics were a miner's canary for pademelon's trouble with post-de6fd1c
inlining.  Expect pademelon to break whenever a frontend-included file gains
an inline function that calls a backend function.  Atomics were the initial
examples, but this patch adds more:

$ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1'
pg_resetxlog.o: In function `fastgetattr':
/data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754:
 undefined reference to `nocachegetattr'
pg_resetxlog.o: In function `heap_getattr':
/data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783:
 undefined reference to `heap_getsysattr'

The htup_details.h case is trickier in a way, because pg_resetxlog has a
legitimate need for SizeofHeapTupleHeader via TOAST_MAX_CHUNK_SIZE.  Expect
this class of problem to recur as we add more inline functions.  Our method to
handle these first two instances will set a precedent.

That gave me new respect for STATIC_IF_INLINE.  While it does add tedious work
to the task of introducing a new batch of inline functions, the work is
completely mechanical.  Anyone can write it; anyone can review it; there's one
correct way to write it.  Header surgery like
0001-Don-t-include-low-level-locking-code-from-frontend-c.patch requires
expert design from scratch, and it (trivially) breaks builds for a sample of
non-core code.  Let's return to STATIC_IF_INLINE and convert fastgetattr()
therewith.  (A possible future line of inquiry is to generate the
STATIC_IF_INLINE transformation at build time, so the handwritten headers can
use C99 inlining directly as though it is always available.)

Thanks,
nm


-- 
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] [RFC] allow 'semester' in functions EXTRACT, DATE_PART, TO_CHAR and so on.

[David Fetter]

On Fri, Aug 14, 2015 at 10:06:22PM -0300, Dickson S. Guedes wrote:
> 2015-08-14 21:32 GMT-03:00 Gavin Flower :
> ...
> > So semesters don't appear to align with normal half year boundaries.
> 
> Interesting links, thanks!
> 
> Which sounds better for a native English: 'half', 'halfyear'?
> 
> For example:
> 
> > SELECT date_trunc('halfyear', current_date);
>date_trunc
> 
>  2015-07-01 00:00:00-03
> (1 row)
> 
> Thanks!

As with, "quarter," we need to be careful about which type of "half"
we are defining.  For some financial calculations, a "month" is always
30 days, a "quarter," always three "months," i.e. 90 days, and a
"half" is six "months," or 180 days.

Yes, date math is crazy, and yes, we have to deal with it as it
exists.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

[HACKERS] Add support for RADIUS passwords longer than 16 octets

[Marko Tiikkaja]

Hi,

The attached patch adds support for RADIUS passwords longer than 16 octets.


.m
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
***
*** 2168,2173  CheckCertAuth(Port *port)
--- 2168,2174 
  
  #define RADIUS_VECTOR_LENGTH 16
  #define RADIUS_HEADER_LENGTH 20
+ #define RADIUS_MAX_PASSWORD_LENGTH 128
  
  typedef struct
  {
***
*** 2241,2247  CheckRADIUSAuth(Port *port)
radius_packet *receivepacket = (radius_packet *) receive_buffer;
int32   service = htonl(RADIUS_AUTHENTICATE_ONLY);
uint8  *cryptvector;
!   uint8   encryptedpassword[RADIUS_VECTOR_LENGTH];
int packetlength;
pgsocketsock;
  
--- 2242,2250 
radius_packet *receivepacket = (radius_packet *) receive_buffer;
int32   service = htonl(RADIUS_AUTHENTICATE_ONLY);
uint8  *cryptvector;
!   int encryptedpasswordlen;
!   uint8   encryptedpassword[RADIUS_MAX_PASSWORD_LENGTH];
!   uint8  *previousxor;
int packetlength;
pgsocketsock;
  
***
*** 2259,2264  CheckRADIUSAuth(Port *port)
--- 2262,2268 
fd_set  fdset;
struct timeval endtime;
int i,
+   j,
r;
  
/* Make sure struct alignment is correct */
***
*** 2316,2325  CheckRADIUSAuth(Port *port)
return STATUS_ERROR;
}
  
!   if (strlen(passwd) > RADIUS_VECTOR_LENGTH)
{
ereport(LOG,
!   (errmsg("RADIUS authentication does not support 
passwords longer than 16 characters")));
return STATUS_ERROR;
}
  
--- 2320,2329 
return STATUS_ERROR;
}
  
!   if (strlen(passwd) > RADIUS_MAX_PASSWORD_LENGTH)
{
ereport(LOG,
!   (errmsg("RADIUS authentication does not support 
passwords longer than %d characters", RADIUS_MAX_PASSWORD_LENGTH)));
return STATUS_ERROR;
}
  
***
*** 2344,2371  CheckRADIUSAuth(Port *port)
radius_add_attribute(packet, RADIUS_NAS_IDENTIFIER, (unsigned char *) 
identifier, strlen(identifier));
  
/*
!* RADIUS password attributes are calculated as: e[0] = p[0] XOR
!* MD5(secret + vector)
 */
!   cryptvector = palloc(RADIUS_VECTOR_LENGTH + 
strlen(port->hba->radiussecret));
memcpy(cryptvector, port->hba->radiussecret, 
strlen(port->hba->radiussecret));
!   memcpy(cryptvector + strlen(port->hba->radiussecret), packet->vector, 
RADIUS_VECTOR_LENGTH);
!   if (!pg_md5_binary(cryptvector, RADIUS_VECTOR_LENGTH + 
strlen(port->hba->radiussecret), encryptedpassword))
!   {
!   ereport(LOG,
!   (errmsg("could not perform MD5 encryption of 
password")));
!   pfree(cryptvector);
!   return STATUS_ERROR;
!   }
!   pfree(cryptvector);
!   for (i = 0; i < RADIUS_VECTOR_LENGTH; i++)
{
!   if (i < strlen(passwd))
!   encryptedpassword[i] = passwd[i] ^ encryptedpassword[i];
else
!   encryptedpassword[i] = '\0' ^ encryptedpassword[i];
}
!   radius_add_attribute(packet, RADIUS_PASSWORD, encryptedpassword, 
RADIUS_VECTOR_LENGTH);
  
/* Length need to be in network order on the wire */
packetlength = packet->length;
--- 2348,2396 
radius_add_attribute(packet, RADIUS_NAS_IDENTIFIER, (unsigned char *) 
identifier, strlen(identifier));
  
/*
!* RADIUS password attributes are calculated as:
!*   e[0] = p[0] XOR MD5(secret + vector)
!* for the first vector, and then:
!*   e[i] = p[i] XOR MD5(secret + p[i-1])
!* for the following ones (if necessary)
 */
!   encryptedpasswordlen = ((strlen(passwd) + RADIUS_VECTOR_LENGTH - 1) / 
RADIUS_VECTOR_LENGTH) * RADIUS_VECTOR_LENGTH;
!   cryptvector = palloc(strlen(port->hba->radiussecret) + 
RADIUS_VECTOR_LENGTH);
memcpy(cryptvector, port->hba->radiussecret, 
strlen(port->hba->radiussecret));
! 
!   for (i = 0; i < encryptedpasswordlen; i += RADIUS_VECTOR_LENGTH)
{
!   if (i == 0)
!   {
!   /* for the first iteration, we use the Request 
Authenticator vector */
!   memcpy(cryptvector + strlen(port->hba->radiussecret), 
packet->vector, RADIUS_VECTOR_LENGTH);
!   }
else
!   {
!   /* .. and for the following iterations the result of 
the previous XOR */
!   memcpy(cryptvector + strlen(port->hba->radiussecret), 
previousxor, RADIUS_VECTOR_LENGTH);
!   }
! 
! 

Re: [HACKERS] replication slot restart_lsn initialization

[Michael Paquier]

On Fri, Aug 14, 2015 at 4:54 PM, Andres Freund  wrote:
> On 2015-08-14 16:44:44 +0900, Michael Paquier wrote:
>> Commit 6fcd8851, which is the result of this thread, is not touching
>> the replication protocol at all. This looks like an oversight to me:
>> we should be a maximum consistent between the SQL interface and the
>> replication protocol if possible, and it looks useful to me to be able
>> to set restart_lsn when creating the slot as well when using a
>> replication connection.
>
> It wasn't, at least not from my side. You can relatively easily do
> nearly the same just by connecting to the slot and sending a feedback
> message. The complaint upthread (and/or a related thread) was that it's
> not possible to do the same from SQL.
>
> It'd be a good additional to offer the same facility to the replication
> protocol.
> [...]
> I'd name it RESERVE_WAL. My feeling is that the options for the logical
> case are geared towards the output plugin, not the walsender. I think
> it'd be confusing to use (slot_options) differently for physical slots.

Well, this has taken less time than I thought:
=# CREATE_REPLICATION_SLOT toto PHYSICAL;
 slot_name | consistent_point | snapshot_name | output_plugin
---+--+---+---
 toto  | 0/0  | null  | null
(1 row)
=# CREATE_REPLICATION_SLOT toto2 PHYSICAL RESERVE_WAL;
 slot_name | consistent_point | snapshot_name | output_plugin
---+--+---+---
 toto2 | 0/0  | null  | null
(1 row)
=# \q
$ psql -c 'select slot_name,restart_lsn from pg_replication_slots'
 slot_name | restart_lsn
---+-
 toto  |
 toto2 | 0/1738850
(2 rows)

What do you want to do with that? Do you wish to have a look at it or
should I register it in the next CF? I am fine with both, though this
is tightly linked with the feature already committed.
Regards,
-- 
Michael
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 42e9497..df49fa3 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1434,7 +1434,7 @@ The commands accepted in walsender mode are:
   
 
   
-CREATE_REPLICATION_SLOT slot_name { PHYSICAL | LOGICAL output_plugin }
+CREATE_REPLICATION_SLOT slot_name { PHYSICAL [ RESERVE_WAL ] | LOGICAL output_plugin }
  CREATE_REPLICATION_SLOT
 
 
@@ -1463,6 +1463,17 @@ The commands accepted in walsender mode are:
  

   
+
+  
+   RESERVE_WAL
+   
+
+ Specify that the LSN for this physical replication
+ slot is reserved immediately; otherwise the LSN is
+ reserved on first connection from a streaming replication client.
+
+   
+  
  
 
   
diff --git a/src/backend/replication/repl_gram.y b/src/backend/replication/repl_gram.y
index e9177ca..077d67d 100644
--- a/src/backend/replication/repl_gram.y
+++ b/src/backend/replication/repl_gram.y
@@ -76,6 +76,7 @@ Node *replication_parse_result;
 %token K_PHYSICAL
 %token K_LOGICAL
 %token K_SLOT
+%token K_RESERVE_WAL
 
 %type 	command
 %type 	base_backup start_replication start_logical_replication
@@ -88,6 +89,7 @@ Node *replication_parse_result;
 %type 	plugin_opt_elem
 %type 	plugin_opt_arg
 %type 		opt_slot
+%type 	opt_reserve_wal
 
 %%
 
@@ -181,13 +183,14 @@ base_backup_opt:
 			;
 
 create_replication_slot:
-			/* CREATE_REPLICATION_SLOT slot PHYSICAL */
-			K_CREATE_REPLICATION_SLOT IDENT K_PHYSICAL
+			/* CREATE_REPLICATION_SLOT slot PHYSICAL RESERVE_WAL */
+			K_CREATE_REPLICATION_SLOT IDENT K_PHYSICAL opt_reserve_wal
 {
 	CreateReplicationSlotCmd *cmd;
 	cmd = makeNode(CreateReplicationSlotCmd);
 	cmd->kind = REPLICATION_KIND_PHYSICAL;
 	cmd->slotname = $2;
+	cmd->reserve_wal = $4;
 	$$ = (Node *) cmd;
 }
 			/* CREATE_REPLICATION_SLOT slot LOGICAL plugin */
@@ -268,6 +271,12 @@ opt_physical:
 			| /* EMPTY */
 			;
 
+opt_reserve_wal:
+			K_RESERVE_WAL
+{ $$ = TRUE }
+			| /* EMPTY */
+{ $$ = FALSE }
+
 opt_slot:
 			K_SLOT IDENT
 { $$ = $2; }
diff --git a/src/backend/replication/repl_scanner.l b/src/backend/replication/repl_scanner.l
index 056cc14..d7030c1 100644
--- a/src/backend/replication/repl_scanner.l
+++ b/src/backend/replication/repl_scanner.l
@@ -95,6 +95,7 @@ CREATE_REPLICATION_SLOT		{ return K_CREATE_REPLICATION_SLOT; }
 DROP_REPLICATION_SLOT		{ return K_DROP_REPLICATION_SLOT; }
 TIMELINE_HISTORY	{ return K_TIMELINE_HISTORY; }
 PHYSICAL			{ return K_PHYSICAL; }
+RESERVE_WAL			{ return K_RESERVE_WAL; }
 LOGICAL{ return K_LOGICAL; }
 SLOT{ return K_SLOT; }
 
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index e1bab07..9142811 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -826,6 +826,14 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
 
 		ReplicationSlotPersist();
 	}
+	else if (cmd-

[HACKERS] Small improvement to get_base_rel_indexes()

[David Rowley]

Attached is a small patch which improves the way get_base_rel_indexes()
works.

The current version creates a new bitmapset on each recursion level then
bms_joins() to the one on the next level up each time. I understand that
this will patch will have about a 0 net performance improvement, but I
thought I'd post anyway as:

1. It removes 5 lines of code.
2. It's a better example to leave in the code.

Is it worth applying?

Regards

David Rowley
--
 David Rowley   http://www.2ndQuadrant.com/

 PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/optimizer/plan/planner.c 
b/src/backend/optimizer/plan/planner.c
index d598c1b..926a41a 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -2638,43 +2638,38 @@ is_dummy_plan(Plan *plan)
  * the only good way to distinguish baserels from appendrel children
  * is to see what is in the join tree.
  */
-static Bitmapset *
-get_base_rel_indexes(Node *jtnode)
+static void
+get_base_rel_indexes(Node *jtnode, Bitmapset **result)
 {
-   Bitmapset  *result;
 
if (jtnode == NULL)
-   return NULL;
+   return;
if (IsA(jtnode, RangeTblRef))
{
int varno = ((RangeTblRef *) 
jtnode)->rtindex;
 
-   result = bms_make_singleton(varno);
+   *result = bms_add_member(*result, varno);
}
else if (IsA(jtnode, FromExpr))
{
FromExpr   *f = (FromExpr *) jtnode;
ListCell   *l;
 
-   result = NULL;
foreach(l, f->fromlist)
-   result = bms_join(result,
- 
get_base_rel_indexes(lfirst(l)));
+   get_base_rel_indexes(lfirst(l), result);
}
else if (IsA(jtnode, JoinExpr))
{
JoinExpr   *j = (JoinExpr *) jtnode;
 
-   result = bms_join(get_base_rel_indexes(j->larg),
- 
get_base_rel_indexes(j->rarg));
+   get_base_rel_indexes(j->larg, result);
+   get_base_rel_indexes(j->rarg, result);
}
else
{
elog(ERROR, "unrecognized node type: %d",
 (int) nodeTag(jtnode));
-   result = NULL;  /* keep compiler quiet */
}
-   return result;
 }
 
 /*
@@ -2684,7 +2679,7 @@ static void
 preprocess_rowmarks(PlannerInfo *root)
 {
Query  *parse = root->parse;
-   Bitmapset  *rels;
+   Bitmapset  *rels = NULL;
List   *prowmarks;
ListCell   *l;
int i;
@@ -2716,7 +2711,7 @@ preprocess_rowmarks(PlannerInfo *root)
 * make a bitmapset of all base rels and then remove the items we don't
 * need or have FOR [KEY] UPDATE/SHARE marks for.
 */
-   rels = get_base_rel_indexes((Node *) parse->jointree);
+   get_base_rel_indexes((Node *) parse->jointree, &rels);
if (parse->resultRelation)
rels = bms_del_member(rels, parse->resultRelation);
 

-- 
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] Test code is worth the space

[Petr Jelinek]

On 2015-08-15 03:35, Jim Nasby wrote:


I setup a simple example of this with 64 variations of TAP tests, BLKSZ
and WAL blocksize. Unfortunately to make this work you have to commit a
.travis.yml file to your fork.

build: https://travis-ci.org/decibel/postgres/builds/75692344
.travis.yml: https://github.com/decibel/postgres/blob/master/.travis.yml

Looks like we might have some problems with BLKSZ != 8...


I went through several of those and ISTM that most test failures are to 
be expected:

a) order of resulting rows is different for some of the joins
b) planner behaves differently because of different number of pages
c) tablesample returns different results because it uses ctids as random 
source
d) toaster behaves differently because of different threshold which is 
calculated from BLKSZ
e) max size of btree value is exceeded with BLKSZ = 4 in the test that 
tries to create deep enough btree


We could fix a) by adding ORDER BY to those queries but I don't see how 
to fix the rest easily or at all without sacrificing some test coverage.


However I see one real error in the BLKSZ = 4 tests - two of the merge 
join queries in equivclass die with "ERROR:  could not find commutator 
for operator" ( example build output 
https://travis-ci.org/decibel/postgres/jobs/75692377 ). I didn't yet 
investigate what's causing this.



--
 Petr Jelinek  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