Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

[Magnus Hagander]

On Tue, Jan 3, 2017 at 4:02 AM, Tom Lane  wrote:

> Peter Eisentraut  writes:
> > Yeah, it seems that if you want to know whether you are using SSL, then
> > we already have that.  I don't see the need for this new read-only
> setting.
>
> I concur --- there might be use for more reporting about SSL status, but
> that patch doesn't seem like quite the right thing.
>
> I've pushed the main patch with some small adjustments; one notable one
> that I made the EXEC_BACKEND reload path work like SIGHUP reload (press on
> without SSL if fail) not server start (abort if fail).  As it stood, if
> you fat-fingered a change to your SSL files on Windows, the postmaster
> would keep running but all backends would die instantly, whether or not
> an SSL connection was being requested.  That didn't seem helpful.
>
> I also went ahead and downgraded the "hostssl record with SSL turned off"
> case to a warning.
>
> Before we leave this area, though, there is a loose end that requires
> more thought.  That is, what about passphrase-protected server keys?
> Our documentation suggests that if you have one, the server will demand
> the passphrase just once at server start and then all is good.  I'm not
> sure if that's at all practical in modern usage, but in any case it's
> not going to be reasonable to put a passphrase in again at every SIGHUP.
> On Windows things are even worse; you'd have to give the passphrase again
> to every spawned backend.  (But that was true already.)
>
> I can think of at least three things we might do about this:
>
> 1. Leave the code as it stands, and change the documentation to state
> that you cannot use a passphrase-protected server key, period.
>

2. Add a password callback function that would supply the passphrase
> when needed.  The question is, where would it get that from?  It'd
> be straightforward to supply it from a string GUC, but from a security
> POV it seems pretty silly to have the password in postgresql.conf.
>


Yeah, that seems like a really bad idea. If you want to do that, then you
might as well just remove the passphrase from the key.



> 3. Add a password callback function that just returns an empty string,
> thereby ensuring a clean failure if the server tries to load a
> passphrase-protected key.  We'd still need to change the documentation
> but at least the behavior would be reasonably clean.
>
> I'm kinda leaning to the last choice; I don't want to leave things as they
> are, but actually making password-protected keys work in a useful way
> seems like a lot more effort than is justified.
>

The effort to deal with it may well be justified. IIRC, Greg Stark had some
ideas of what he wanted to do with encryption keys (or maybe that was
either somebody else or some other keys?), which also included getting the
private key out of the general postmaster address space to protect it. But
it *is* a bigger option, and in particular it is well out of scope of this
patch.

Of the three options I agree 3 is probably the best.

Another option would be to use a callback to get the key value the first
time, and then cache it so we can re-use it. That means we can make it work
if the new key has the same password, but it also means we need to take
care of protecting that passphrase. But I'm not sure that's any worse than
the fact that we keep the private key around unlocked today?

That said, they passphrase should only be required if the key changes, not
if the certificate changes, I believe. Do we take advantage of this today
(sorry, haven't looked at the code)? Because the most common operation is
probably the renewal of a certificate, which does not change the key, for
example...

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

Re: [HACKERS] proposal: session server side variables

[Pavel Stehule]

2017-01-02 16:55 GMT+01:00 Fabien COELHO :

>
> Hello,
>
> In my proposal was support for transaction scope - ON COMMIT RESET clause
 should be ok

>>>
>>> Could you update the wiki, both the proposal and the use-case
>>> implementation, to reflect this point?
>>>
>>> Moreover, is there any actual use-case for non-transactional secure
>>> half-persistent session variables? AFAICS the "secure" part implies both
>>> permissions and transactional for the presented security-related use
>>> case.
>>> If there is no use case for these combined features, then ISTM that you
>>> should update to proposal so that the variables are always transactional,
>>> which is both simpler, more consistent, and I think more acceptable.
>>>
>>
>> If you are transaction sensitive, then you have to be sensitive to
>> subtransactions - then the work is much more complex.
>>
>
> Maybe, probably, I do not really know. For now, I'm trying to determine
> how the proposals fits Craig's use case.
>
> The current status is that both proposals are useless because the use case
> needs "some" transactional property for security. But probably some
> improvements are possible.
>
> Is there use case, when you would to play with transactions and variables
>> and RESET is not enough?
>>
>
> I do not know. If you explain more clearly what is meant by a "RESET" on a
> variable when the transaction fails, then maybe I can have an opinion.
> Currently I'm just guessing in the dark the precise intended semantics.


reset can means "set to default"

Now when I though about it - this scenario is not interesting for PL -
probably can be interesting for some interactive work. In PL you can handle
transactions - so you know if was or was not any exceptions. And if you
didn't handle the exception, then you are in "need rollback state", so you
cannot to anything - look on variable value too. In PL is usually important
transaction start - difficult question if it can means subtransaction start
too.

Regards

Pavel

>
>
> --
> Fabien.
>

Re: [HACKERS] postgres_fdw : altering foreign table not invalidating prepare statement execution plan.

[Ashutosh Bapat]

On Wed, Nov 30, 2016 at 2:35 PM, Etsuro Fujita
 wrote:
> On 2016/11/30 17:53, Amit Langote wrote:
>>
>> On 2016/11/30 17:25, Etsuro Fujita wrote:
>>>
>>> Done.  I modified the patch so that any inval in pg_foreign_server also
>>> blows the whole plan cache.
>
>
>> I noticed the following addition:
>>
>> +   CacheRegisterSyscacheCallback(FOREIGNDATAWRAPPEROID,
>> PlanCacheSysCallback, (Datum) 0);
>>
>> Is that intentional?  I thought you meant only to add for
>> pg_foreign_server.
>
>
> Yes, that's intentional; we would need that as well, because cached plans
> might reference FDW-level options, not only server/table-level options.  I
> couldn't come up with regression tests for FDW-level options in
> postgres_fdw, though.


The patch looks good to me, but I feel there are too many testscases.
Now that we have changed the approach to invalidate caches in all
cases, should we just include cases for SELECT or UPDATE or INSERT or
DELETE instead of each statement?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] Cache Hash Index meta page.

[Mithun Cy]

Thanks Amit for detailed review, and pointing out various issues in
the patch. I have tried to fix all of your comments as below.

On Mon, Jan 2, 2017 at 11:29 AM, Amit Kapila  wrote:

> 1.
> usage "into to .." in above comment seems to be wrong.usage "into to .." in 
> above comment seems to be wrong.usage "into to .." in above comment seems to 
> be wrong.usage "into to .." in above comment seems to be wrong.

-- Fixed.

> 2.
> In the above comment, a reference to HashMetaCache is confusing, are
> your referring to any structure here?  If you change this, consider toyour 
> referring to any structure here?  If you change this, consider toyour 
> referring to any structure here?  If you change this, consider toyour 
> referring to any structure here?  If you change this, consider to
> change the similar usage at other places in the patch as well.change the 
> similar usage at other places in the patch as well.change the similar usage 
> at other places in the patch as well.change the similar usage at other places 
> in the patch as well.

-- Fixed. Removed HashMetaCache everywhere in the code. Where ever
needed added HashMetaPageData.

> Also, it is not clear to what do you mean by ".. to indicate bucketto 
> indicate bucket
> has been initialized .."?  assigning maxbucket as a special value tohas been 
> initialized .."?  assigning maxbucket as a special value to
> prevblkno is not related to initializing a bucket page.prevblkno is not 
> related to initializing a bucket page.

-- To be consistent with our definition of prevblkno, I am setting
prevblkno with current hashm_maxbucket when we initialize the bucket
page. I have tried to correct the comments accordingly.

> 3.
> In above comment, where you are saying ".. caching the some of the
> meta page data .." is slightly confusing, because it appears to me
> that you are caching whole of the metapage not some part of it.

-- Fixed. Changed to caching the HashMetaPageData.

> 4.
> +_hash_getbucketbuf_from_hashkey(uint32 hashkey, Relation rel,
>
> Generally, for _hash_* API's, we use rel as the first parameter, so I
> think it is better to maintain the same with this API as well.

-- Fixed.

> 5.
>   _hash_finish_split(rel, metabuf, buf, pageopaque->hasho_bucket,
> -   maxbucket, highmask, lowmask);
> +   usedmetap->hashm_maxbucket,
> +   usedmetap->hashm_highmask,
> +   usedmetap->hashm_lowmask);

> I think we should add an Assert for the validity of usedmetap before using it.

-- Fixed. Added Assert(usedmetap != NULL);

> 6. Getting few compilation errors in assert-enabled build.
>

-- Fixed. Sorry, I missed handling bucket number which is needed at
below codes. I have fixed same now.

> 7.
> I can understand what you want to say in above comment, but I think
> you can write it in somewhat shorter form.  There is no need to
> explain the exact check.

-- Fixed. I have tried to compress it into a few lines.

> 8.
> @@ -152,6 +152,11 @@ _hash_readprev(IndexScanDesc scan,
>   _hash_relbuf(rel, *bufp);
>
>   *bufp = InvalidBuffer;
> +
> + /* If it is a bucket page there will not be a prevblkno. */
> + if ((*opaquep)->hasho_flag & LH_BUCKET_PAGE)
> + return;
> +
>
> I don't think above check is right.  Even if it is a bucket page, we
> might need to scan bucket being populated, refer check else if
> (so->hashso_buc_populated && so->hashso_buc_split).  Apart from that,
> you can't access bucket page after releasing the lock on it.  Why have
> you added such a check?
>

-- Fixed. That was a mistake, now I have fixed it. Actually, if bucket
page is passed to _hash_readprev then there will not be a prevblkno.
But from this patch onwards prevblkno of bucket page will store
hashm_maxbucket. So a check BlockNumberIsValid (blkno) will not be
valid anymore. I have fixed by adding as below.
+ /* If it is a bucket page there will not be a prevblkno. */
+ if (!((*opaquep)->hasho_flag & LH_BUCKET_PAGE))
  {
+ Assert(BlockNumberIsValid(blkno));

There are 2 other places which does same @_hash_freeovflpage,
@_hash_squeezebucket.
But that will only be called for overflow pages. So I did not make
changes. But I think we should also change there to make it
consistent.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


cache_hash_index_meta_page_09.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] Potential data loss of 2PC files

[Ashutosh Bapat]

On Sat, Dec 31, 2016 at 5:53 AM, Michael Paquier
 wrote:
> On Fri, Dec 30, 2016 at 10:59 PM, Ashutosh Bapat
>  wrote:
>>>
>>> Well, flushing the meta-data of pg_twophase is really going to be far
>>> cheaper than the many pages done until CheckpointTwoPhase is reached.
>>> There should really be a check on serialized_xacts for the
>>> non-recovery code path, but considering how cheap that's going to be
>>> compared to the rest of the restart point stuff it is not worth the
>>> complexity of adding a counter, for example in shared memory with
>>> XLogCtl (the counter gets reinitialized at each checkpoint,
>>> incremented when replaying a 2PC prepare, decremented with a 2PC
>>> commit). So to reduce the backpatch impact I would just do the fsync
>>> if (serialized_xact > 0 || RecoveryInProgress()) and call it a day.
>>
>> Sounds ok to me. I will leave it to the committer to decide further.
>
> Attached is an updated patch. I also noticed that it is better to do
> the fsync call before the dtrace call for checkpoint end as well as
> logging.

The patch looks good to me.

I am wondering what happens if a 2PC file gets created, at the time of
checkpoint we flush the pg_twophase directory, then the file gets
removed. Do we need to flush the directory to ensure that the removal
persists? Whatever material I look for fsync() on directory, it gives
examples of file creation, not that of deleting a file. If we want to
persist the removal, probably we have to flush pg_twophase always or
add code to track whether any activity happened in pg_twophase between
two checkpoints. The later seems complication not worth the benefit.

I guess, it's hard to construct a case to reproduce the issue
described in your first mail. But still checking if you have any
reproduction. May be we could use similar reproduction to test the
deletion of two phase file.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] background sessions

[amul sul]

On Fri, Dec 30, 2016 at 3:48 AM, Peter Eisentraut
 wrote:
> On 12/16/16 10:38 AM, Andrew Borodin wrote:
>> 2016-12-16 20:17 GMT+05:00 Peter Eisentraut 
>> :
 And one more thing... Can we have BackgroundSessionExecute() splitted
 into two parts: start query and wait for results?
 It would allow pg_background to reuse bgsession's code.
>>>
>>> Yes, I will look into that.
>>
>> Thank you. I'm marking both patches as "Waiting for author", keeping
>> in mind that pg_background is waiting for bgsessions.
>> After updates I'll review these patches.
>
> New patch, mainly with the function split as you requested above, not
> much else changed.
>

Thanks for your v2 patch, this is really helpful.

One more requirement for pg_background is session, command_qh,
response_qh and worker_handle should be last longer than current
memory context, for that we might need to allocate these in
TopMemoryContext.  Please find attach patch does the same change in
BackgroundSessionStart().

Do let me know if you have any other thoughts/suggestions, thank you.

Regards,
Amul


BackgroundSessionStart.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] What is "index returned tuples in wrong order" for recheck supposed to guard against?

[Regina Obe]

>> If things are out of order, why isn't just going to was_exact = false 
>> good enough?
>>
>> I'm not sure if the mistake is in our PostGIS code or something in 
>> PostgreSQL recheck logic.
>> If I change the elog(ERROR ...) to a elog(NOTICE, the answers  are 
>> correct and sort order is right.
>>
>> Under what conditions would cmp return less than 0?  I tried following 
>> the code in cmp_orderbyvals, but got lost and trying to put elog 
>> notices in to see what the distance is returning (I probably did it 
>> wrong), just ended up crashing by backend.

> cmp would return 0 if the estimated distance returned by the index AM were 
> greater than the actual distance.  
> The estimated distance can be less than the actual distance, but it isn't 
> allowed to be more.  See gist_bbox_distance for an example of a "lossy" 
> distance calculation, and more generally "git show 
> 35fcb1b3d038a501f3f4c87c05630095abaaadab".

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

Did you mean would return < 0 ? 

Since I thought 0 meant exact and not where it's Erroring?

I think for points then maybe we should turn it off, as this could just be 
floating point issues with the way we compute the index.
That would explain why it doesn't happen for other cases like  polygon / point 
in our code
or polygon /polygon in our code since the box box distance in our code would 
always be <= actual distance for those.

So maybe the best course of action is just for us inspect the geometries and if 
both are points just disable recheck.

It's still not quite clear to me even looking at that git commit, why those 
need to error instead of going thru recheck aside from efficiency.


Thanks,
Regina



-- 
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] safer node casting

[Ashutosh Bapat]

On Mon, Jan 2, 2017 at 2:10 PM, Andres Freund  wrote:
> Hi,
>
>
> On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
>> There is a common coding pattern that goes like this:
>>
>> RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
>> Assert(IsA(rinfo, RestrictInfo));
>
>
>> I propose a macro castNode() that combines the assertion and the cast,
>> so this would become
>>
>> RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));
>
> I'm quite a bit in favor of something like this, having proposed it
> before ;)
>
>> +#define castNode(_type_,nodeptr) (AssertMacro(!nodeptr || 
>> IsA(nodeptr,_type_)), (_type_ *)(nodeptr))
>
> ISTM that we need to do the core part of this in an inline function, to
> avoid multiple evaluation hazards - which seem quite likely to occur
> here - it's pretty common to cast the result of a function after all.
>
> Something like
>
> static inline Node*
> castNodeImpl(void *c, enum NodeTag t)
> {
> Assert(c == NULL || IsA(c, t));
> return c;
> }
>
> #define castNode(_type_, nodeptr) ((_type_ *) castNodeImpl(nodeptr, _type_))
>
> should work without too much trouble afaics?
>
I tried this quickly as per attached patch. It gave a compiler error
createplan.c: In function ‘castNodeImpl’:
createplan.c:340:2: error: ‘T_t’ undeclared (first use in this function)
createplan.c:340:2: note: each undeclared identifier is reported only
once for each function it appears in
createplan.c: In function ‘create_plan_recurse’:
createplan.c:445:13: error: expected expression before ‘AggPath’

Is the attached patch as per your suggestion?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


castNode.patch
Description: application/download

-- 
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] Add support to COMMENT ON CURRENT DATABASE

[Ashutosh Bapat]

The patch has white space error
git apply /mnt/hgfs/tmp/comment_on_current_database_v1.patch
/mnt/hgfs/tmp/comment_on_current_database_v1.patch:52: trailing whitespace.
 * schema-qualified or catalog-qualified.
warning: 1 line adds whitespace errors.

The patch compiles clean, regression is clean. I tested auto
completion of current database, as well pg_dumpall output for comments
on multiple databases. Those are working fine. Do we want to add a
testcase for testing the SQL functionality as well as pg_dumpall
functionality?

Instead of changing get_object_address_unqualified(),
get_object_address_unqualified() and pg_get_object_address(), should
we just stick get_database_name(MyDatabaseId) as object name in
gram.y? That would be much cleaner than special handling of NIL list.
It looks dubious to set that list as NIL when the target is some
object. If we do that, we will spend extra cycles in finding database
id which is ready available as MyDatabaseId, but the code will be
cleaner. Another possibility is to use objnames to store the database
name and objargs to store the database id. That means we overload
objargs, which doesn't looks good either.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] [PATCH] PostgresNode.pm enhancements, pg_lsn helper, and some more recovery tests

[Craig Ringer]

On 2 January 2017 at 20:17, Simon Riggs  wrote:

> Bit confused... can't see a caller for wait_for_slot_catchup() and the
> slot tests don't call it AFAICS.

The recovery tests for decoding on standby will use it. I can delay
adding it until then.

> Also can't see anywhere the LSN stuff is used either.

Removed after discussion with Michael in the logical decoding on standby thread.

I'll be posting a new patch series there shortly, which removes
pg_lsn.pm and modifies the PostgresNode.pm changes per discussion. If
you're able to commit the updated PostgresNode.pm and new standby
tests that'd be very handy.

-- 
 Craig Ringer   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] Odd behavior with PG_TRY

[Tom Lane]

Jim Nasby  writes:
> In the attached patch (snippet below), I'm seeing something strange with 
> args->in.r.atts[].

Did you try comparing the apparent values of "args" before and after
entering PG_TRY?

> I saw the comment on PG_TRY about marking things as volatile, but my 
> understanding from the comment is I shouldn't even need to do that, 
> since these variables aren't being modified.

Not sure, but if you do need to mark them volatile to prevent
misoptimization in the PG_TRY, this is not how to do it:

> volatile TupleDescdesc = slot->tts_tupleDescriptor;
> volatile CallbackState *myState = (CallbackState *) self;
> volatile PLyTypeInfo *args = myState->args;

Correct coding would be

volatile TupleDesc  desc = slot->tts_tupleDescriptor;
CallbackState * volatile myState = (CallbackState *) self;
PLyTypeInfo * volatile args = myState->args;

because what needs to be marked volatile is the pointer variable,
not what it points at.  I'm a bit surprised you're not getting
"cast away volatile" warnings from the code as you have 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] Odd behavior with PG_TRY

[Amit Kapila]

On Mon, Jan 2, 2017 at 10:43 PM, Jim Nasby  wrote:
> On 1/2/17 1:31 AM, Amit Kapila wrote:
>>
>> On Mon, Jan 2, 2017 at 11:14 AM, Jim Nasby 
>> wrote:
>>
>> Looks strange, what is the value of 'i'?  Did you get the same result
>> if you try to print args->in.r.atts[0] inside PG_TRY?
>
>
> i is 0, makes no difference:
>
> (lldb) call i
> (int) $56 = 0
> (lldb) call args->in.r.atts[0]
> error: Couldn't apply expression side effects : Couldn't dematerialize a
> result variable: couldn't read its memory
> (lldb)
>

To localize the problem you might want to try by just having the
problematic statement in PG_TRY();

PG_TRY();
{
if (args->in.r.atts[0].func == NULL)
{
}
}
PG_CATCH();
{
PG_RE_THROW();
}
PG_END_TRY();



-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
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] PassDownLimitBound for ForeignScan/CustomScan [take-2]

[Kohei KaiGai]

Oops, I oversight this patch was marked as "returned with feedback",
not "moved to the next CF".

Its status has not been changed since the last update. (Code was
revised according to the last
comment by Jeevan, but CF-Nov was time up at that time.)

How do I handle the patch?

2016-12-05 16:49 GMT+09:00 Kouhei Kaigai :
> Hello,
>
> Sorry for my late response.
> The attached patch reflects your comments.
>
>> Here are few comments on latest patch:
>>
>>
>> 1.
>> make/make check is fine, however I am getting regression failure in
>> postgres_fdw contrib module (attached regression.diff).
>> Please investigate and fix.
>>
> It was an incorrect interaction when postgres_fdw tries to push down
> sorting to the remote side. We cannot attach LIMIT clause on the plain
> scan path across SORT, however, the previous version estimated the cost
> for the plain scan with LIMIT clause even if local sorting is needed.
> If remote scan may return just 10 rows, estimated cost of the local sort
> is very lightweight, thus, this unreasonable path was chosen.
> (On the other hands, its query execution results were correct because
> ps_numTuples is not delivered across Sort node, so ForeignScan eventually
> scanned all the remote tuples. It made correct results but not optimal
> from the viewpoint of performance.)
>
> The v3 patch estimates the cost with remote LIMIT clause only if supplied
> pathkey strictly matches with the final output order of the query, thus,
> no local sorting is expected.
>
> Some of the regression test cases still have different plans but due to
> the new optimization by remote LIMIT clause.
> Without remote LIMIT clause, some of regression test cases preferred
> remote-JOIN + local-SORT then local-LIMIT.
> Once we have remote-LIMIT option, it allows to discount the cost for
> remote-SORT by choice of top-k heap sorting.
> It changed the optimizer's decision on some test cases.
>
> Potential one big change is the test case below.
>
>  -- CROSS JOIN, not pushed down
>  EXPLAIN (VERBOSE, COSTS OFF)
>  SELECT t1.c1, t2.c1 FROM ft1 t1 CROSS JOIN ft2 t2 ORDER BY t1.c1, t2.c1 
> OFFSET 100 LIMIT 10;
>
> It assumed CROSS JOIN was not pushed down due to the cost for network
> traffic, however, remote LIMIT reduced the estimated number of tuples
> to be moved. So, all of the CROSS JOIN + ORDER BY + LIMIT became to run
> on the remote side.
>
>> 2.
>> + *
>> + * MEMO: root->limit_tuples is not attached when query
>> contains
>> + * grouping-clause or aggregate functions. So, we don's adjust
>> + * rows even if LIMIT  is supplied.
>>
>> Can you please explain why you are not doing this for grouping-clause or
>> aggregate functions.
>>
> See grouping_planner() at optimizer/plan/planner.c
> It puts an invalid value on the root->limit_tuples if query has GROUP BY 
> clause,
> so we cannot know number of tuples to be returned even if we have upper limit
> actually.
>
> /*
>  * Figure out whether there's a hard limit on the number of rows that
>  * query_planner's result subplan needs to return.  Even if we know a
>  * hard limit overall, it doesn't apply if the query has any
>  * grouping/aggregation operations, or SRFs in the tlist.
>  */
> if (parse->groupClause ||
> parse->groupingSets ||
> parse->distinctClause ||
> parse->hasAggs ||
> parse->hasWindowFuncs ||
> parse->hasTargetSRFs ||
> root->hasHavingQual)
> root->limit_tuples = -1.0;
> else
> root->limit_tuples = limit_tuples;
>
>> 3.
>> Typo:
>>
>> don's  => don't
>>
> Fixed,
>
> best regards,
> 
> PG-Strom Project / NEC OSS Promotion Center
> KaiGai Kohei 
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



-- 
KaiGai Kohei 


passdown-limit-fdw.v3.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] [PATCH] Reload SSL certificates on SIGHUP

[Tom Lane]

Peter Eisentraut  writes:
> Yeah, it seems that if you want to know whether you are using SSL, then
> we already have that.  I don't see the need for this new read-only setting.

I concur --- there might be use for more reporting about SSL status, but
that patch doesn't seem like quite the right thing.

I've pushed the main patch with some small adjustments; one notable one
that I made the EXEC_BACKEND reload path work like SIGHUP reload (press on
without SSL if fail) not server start (abort if fail).  As it stood, if
you fat-fingered a change to your SSL files on Windows, the postmaster
would keep running but all backends would die instantly, whether or not
an SSL connection was being requested.  That didn't seem helpful.

I also went ahead and downgraded the "hostssl record with SSL turned off"
case to a warning.

Before we leave this area, though, there is a loose end that requires
more thought.  That is, what about passphrase-protected server keys?
Our documentation suggests that if you have one, the server will demand
the passphrase just once at server start and then all is good.  I'm not
sure if that's at all practical in modern usage, but in any case it's
not going to be reasonable to put a passphrase in again at every SIGHUP.
On Windows things are even worse; you'd have to give the passphrase again
to every spawned backend.  (But that was true already.)

I can think of at least three things we might do about this:

1. Leave the code as it stands, and change the documentation to state
that you cannot use a passphrase-protected server key, period.

2. Add a password callback function that would supply the passphrase
when needed.  The question is, where would it get that from?  It'd
be straightforward to supply it from a string GUC, but from a security
POV it seems pretty silly to have the password in postgresql.conf.

3. Add a password callback function that just returns an empty string,
thereby ensuring a clean failure if the server tries to load a
passphrase-protected key.  We'd still need to change the documentation
but at least the behavior would be reasonably clean.

I'm kinda leaning to the last choice; I don't want to leave things as they
are, but actually making password-protected keys work in a useful way
seems like a lot more effort than is justified.

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] What is "index returned tuples in wrong order" for recheck supposed to guard against?

[Robert Haas]

On Fri, Dec 30, 2016 at 12:51 AM, Regina Obe  wrote:
> I've been trying to troubleshoot the cause of this PostGIS recheck bug we
> have reported by two people so far.  The last test was a nice simple
> repeatable one that triggered the issue:
>
> https://trac.osgeo.org/postgis/ticket/3418
>
> from what I have seen this only affects cases where we are doing a distance
> check between two points, which we actually don't need to enable recheck for
> anyway, but trying to disable that seems like just shoving the real problem
> under the covers.

Agreed.

> If things are out of order, why isn't just going to was_exact = false good
> enough?
>
> I'm not sure if the mistake is in our PostGIS code or something in
> PostgreSQL recheck logic.
> If I change the elog(ERROR ...) to a elog(NOTICE, the answers  are correct
> and sort order is right.
>
> Under what conditions would cmp return less than 0?  I tried following the
> code in cmp_orderbyvals, but got lost
> and trying to put elog notices in to see what the distance is returning (I
> probably did it wrong), just ended up crashing by backend.

cmp would return 0 if the estimated distance returned by the index AM
were greater than the actual distance.  The estimated distance can be
less than the actual distance, but it isn't allowed to be more.  See
gist_bbox_distance for an example of a "lossy" distance calculation,
and more generally "git show
35fcb1b3d038a501f3f4c87c05630095abaaadab".

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

[HACKERS] Causal reads take II

[Thomas Munro]

Hi hackers,

Here is a new version of my "causal reads" patch (see the earlier
thread from the 9.6 development cycle[1]), which provides a way to
avoid stale reads when load balancing with streaming replication.

To try it out:

Set up a primary and some standbys, and put "causal_reads_timeout =
4s" in the primary's postgresql.conf.  Then SET causal_reads = on and
experiment with various workloads, watching your master's log and
looking at pg_stat_replication.  For example you could try out
test-causal-reads.c with --causal-reads --check (from the earlier
thread) or write something similar, and verify the behaviour while
killing, pausing, overwhelming servers etc.

Here's a brief restatement of the problem I'm trying to address and
how this patch works:

In 9.6 we got a new synchronous_commit level "remote_apply", which
causes committing transactions to block until the commit record has
been applied on the current synchronous standby server.  In 10devel
can now be servers plural.  That's useful because it means that a
client can run tx1 on the primary and then run tx2 on an appropriate
standby, or cause some other client to do so, and be sure that tx2 can
see tx1.  Tx2 can be said to be "causally dependent" on tx1 because
clients expect tx2 to see tx1, because they know that tx1 happened
before tx2.

In practice there are complications relating to failure and
transitions.  How should you find an appropriate standby?  Suppose you
have a primary and N standbys, you set synchronous_standby_names to
wait for all N standbys, and you set synchronous_commit to
remote_apply.  Then the above guarantee of visibility of tx1 by tx2
works, no matter which server you run tx2 on.  Unfortunately, if one
of your standby servers fails or there is a network partition, all
commits will block until you fix that.  So you probably want to set
synchronous_standby_names to wait for a subset of your set of
standbys.  Now you can lose some number of standby servers without
holding up commits on the primary, but the visibility guarantee for
causal dependencies is lost!  How can a client know for certain
whether tx2 run on any given standby can see a transaction tx1 that it
has heard about?  If you're using the new "ANY n" mode then the subset
of standbys that have definitely applied tx1 is not known to any
client; if you're using the traditional FIRST mode it's complicated
during transitions (you might be talking to a standby that has
recently lost its link to the primary and the primary could have
decided to wait for the next highest priority standby instead and then
returned from COMMIT successfully).

This patch provides the following guarantee:  if causal_reads is on
for both tx1 and tx2, then after tx1 returns, tx2 will either see tx1
or fail with an error indicating that the server is currently
unavailable for causal reads.  This guarantee is upheld even if there
is a network partition and the standby running tx2 is unable to
communicate with the primary server, but requires the system clocks of
all standbys to differ from the primary's by less than a certain
amount of allowable skew that is accounted for in the algorithm
(causal_reads_timeout / 4, see README.causal_reads for gory details).

It works by sending a stream of "leases" to standbys that are applying
fast enough.  These leases grant the standby the right to assume that
all transactions that were run with causal_reads = on and have
returned control have already been applied locally, without doing any
communication or waiting, for a limited time.  Leases are promises
made by the primary that it will wait for all such transactions to be
applied on each 'available' standby or for available standbys' leases
to be revoked because they're lagging too much, and for any revoked
leases to expire.

As discussed in the earlier thread, there are other ways that tx2 run
on a standby could get a useful guarantee about the visibility of an
early transaction tx1 that the client knows about.  (1)  User-managed
"causality tokens":  Clients could somehow obtain the LSN of commit
tx1 (or later), and then tx2 could explicitly wait for that LSN to be
applied, as proposed by Ivan Kartyshov[2] and others; if you aren't
also using sync rep for data loss avoidance, then tx1 will return from
committing without waiting for standbys, and by the time tx2 starts on
a standby it may find that the LSN has already been applied and not
have to wait at all.  That is definitely good.  Unfortunately it also
transfers the problem of tracking causal dependencies between
transactions to client code, which is a burden on the application
developer and difficult to retrofit.  (2)  Middleware-managed
causality tokens:  Something like pgpool or pgbouncer or some other
proxy could sit in front of all of your PostgreSQL servers and watch
all transactions and do the LSN tracking for you, inserting waits
where appropriate so that no standby query ever sees a snapshot that
doesn't include any commit that any client 

Re: [HACKERS] Cluster wide option to control symbol case folding

[Lewis, Ian (Microstar Laboratories)]

From: Robert Haas [mailto:robertmh...@gmail.com] wrote:

> I'm not sure there's any way to split the baby here: tool authors will
obviously prefer that PostgreSQL's behavior in this area be invariable,
while people trying to develop portable database applications will
prefer configurability.
> As far as I can see, this is a zero sum game that is bound to have one
winner and one loser.

Tom is clearly right that such modes make life harder in a fundamental
way for anyone writing only against PostgreSQL. And, excepting the upper
case folding option, which is of no interest at all to me personally - I
do not care which case folding messes up my symbol declarations, it
would move PostgreSQL away from the standard rather than closer to it
(against that, however, PostgreSQL has many features that are not part
of the standard, including its existing lower case folding).

If he is also right that addition of such an option would deteriorate
into a situation where more people think PostgreSQL is broken, rather
than fewer people thinking that, as I think would be the case, I have no
strong argument for why PostgreSQL - as a project - should support such
modal behavior. 

Personally, I believe such an option would increase, not decrease the
number of people who could relatively easily use PostgreSQL. If that is
right it is a strong argument for such a modal behavior in spite of the
obvious real pain. 

And, from what I can see, many, maybe most, general purpose tool authors
target many backends. So, they already have to deal with some
signficiant degree of variation in case folding behavior.

So, I do not really see this as a zero sum game. It is a question of
whether such an option would grow the user base. If not, it is clearly a
bad idea for the project. But, if it would grow the user base
sufficiently, then, yes, there is pain for those who write general
purpose tools aimed only at PostgreSQL. But, such tools gain from a
wider adoption of PostgreSQL.

Ian Lewis (www.mstarlabs.com)


-- 
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] Measuring replay lag

[Thomas Munro]

On Thu, Dec 29, 2016 at 1:28 AM, Thomas Munro
 wrote:
> On Thu, Dec 22, 2016 at 10:14 AM, Thomas Munro
>  wrote:
>> On Thu, Dec 22, 2016 at 2:14 AM, Fujii Masao  wrote:
>>> I agree that the capability to measure the remote_apply lag is very useful.
>>> Also I want to measure the remote_write and remote_flush lags, for example,
>>> in order to diagnose the cause of replication lag.
>>
>> Good idea.  I will think about how to make that work.
>
> Here is an experimental version that reports the write, flush and
> apply lag separately as requested.  This is done with three separate
> (lsn, timestamp) buffers on the standby side.  The GUC is now called
> replication_lag_sample_interval.  Not tested much yet.

Here is a new version that is slightly refactored and fixes a problem
with stale samples after periods of idleness.

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


replay-lag-v16.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] Cluster wide option to control symbol case folding

[Lewis, Ian (Microstar Laboratories)]

gsst...@gmail.com [mailto:gsst...@gmail.com] On Behalf Of Greg Stark
wrote:

> But the problem with configurable quoting rules is a bit different.
> Imagine your application later decides to depend on PostGIS. So you
load the PostGIS extension and perhaps also some useful functions you
found on Stack Overflow for solving some GIS problem you have. Those
extensions will create objects and then work with those objects and may
use CamelCase for clarity -- in > fact I think PostGIS functions are
documented as CamelCase.  The PostGIS extensions might not work on your
system with different case rules if they haven't been 100% consistent
with their camelCasing, and the functions from StackOverflow would be
even less likely to work.

Well, in the case of StackOverflow suggestions, I cannot remember a time
when I did not have to rewrite whatever suggestions I have found and
used. That is not to say that StackOverflow is not useful. It is
incredibly useful at times. But, the suggestions are usually fragments
showing how to do something, not solutions. And, most such suggestions
are small. And, so, relatively easy to understand and patch as needed.
Many such suggestions are not very useful verbatim anyhow. They are
useful exactly because they allow you to understand something that you
were unable to glean from the documentation. Certainly, making symbol
usage consistent is not a hard patch on a small fragment of code that
probably needs help anyhow to bring it to production grade. I would not
consider this a strong argument against having modal symbol recognition.

Your point about PostGIS, and other full or partial solutions for a
complex problem, is a more serious issue. I do not have a strong answer
to this point. However, at the least a CamelCase case defect in a tool
is a pretty easy problem to locate and submit as a patch. (I understand
that your point is not just about PostGIS, but for PostGIS itself I have
read in a few places that they quote everything already. I do not know
whether that is true or not as I have never even looked at the tool.
However, if it is true they quote everything, then they already have
their CamelCase exactly right everywhere. If they did not the symbol
lookup would fail against current PostgreSQL. Any tool that quotes
everything should work the same way against any mode as long as all
modes are case sensitive. It might be ugly, but at least it should
always work no matter what the back end case translation.)

In our own code, I actually would prefer that we were forced to always
use the same case everywhere we refer to a symbol. And a case sensitive
behavior would enforce that at testing. I do not want this because I
want to be able to define symbols that differ only in case. I want it so
that every symbol reference is exactly visually like every other symbol
reference to the same object. Even though the effect is small, I think
such consistency makes it easier to read code. Even in C we almost never
use the ability to overload on case alone except in a few rare - and
localized - cases where the code is actually clearer with such a
notation. For example, in a mathematical implementation, using a
notation where something like t acts as an index and T defines the range
of t the difference in case is very clear. Perhaps more importantly,
this use of overload on case is consistent with conventional
mathematical notation (which, in my opinion is very good where it
belongs). This is not true when dealing with TheLongSymbolWithMixedCase
vs. TheLongSymbolWithMixedcase. The human brain cannot see that
difference easily, while it can see the difference between t and T very
easily, and it can see the relationship between the two symbols more
easily than it can see the relationship between t and tmax, say. Still,
we almost never have such code running on a database server.

Anyhow, you have a good point about third party libraries and tools that
integrate with PostgreSQL. However, I for one would be willing to live
with and address that kind of issue as needed. If the behavior were
controlled at database create time, which, from the articles Tom linked,
seems to be the general consensus as the right time for such a choice
given the current implementation, then one would at least have the
option of having databases with different case rules within a cluster.
Since each session can only connect to one database, this is not a
solution to every such situation, but it would address at least some
such cases.

Ian Lewis (www.mstarlabs.com)

PS. To anyone who might know the answer: My Reply All to this group does
not seem to join to the original thread. All I am doing is Reply All
from Outlook. Is there something else I need to do to allow my responses
to join the original thread?


-- 
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 .. ALTER COLUMN .. ERROR: attribute .. has wrong type

[Justin Pryzby]

On Wed, Oct 12, 2016 at 10:25:05AM -0500, Justin Pryzby wrote:
> > I don't have a clear recollection how I solved this in July; possibly by
> > restoring the (historic, partition) table from backup.
> > 
> > Last week again again just now (both under 9.6), a colleague found that he 
> > was
> > able to avoid the error by ALTER TYPE without USING.
> > 
> > Note that we ALTER TABLE .. NO INHERIT the partitions for all but the most
> > recent 2 months before ALTERing them (or the parent).  The "ALTER NO 
> > INHERIT"
> > and the ALTER TYPE of historic partitions are done outside of a transaction 
> > in
> > order to avoid large additional disk use otherwise used when ALTERing a 
> > parent
> > with many or large children (the sum of the size of the children).

Here's DETAILs for a 2nd such error which has shown up today:

(EricssonUtranXmlParser): Failed to alter table 
eric_umts_rnc_utrancell_metrics: ERROR:  attribute 424 has wrong type
DETAIL:  Table has type smallint, but query expects integer.

(EricssonUtranXmlParser): Failed to alter table 
eric_umts_rnc_utrancell_metrics: ERROR:  attribute 361 has wrong type
DETAIL:  Table has type integer, but query expects smallint.

Also, note both alters really do work without "USING":

ts=# begin; drop view umts_eric_ch_switch_view, eric_umts_rnc_utrancell_view, 
umts_eric_cell_integrity_view; ALTER TABLE eric_umts_rnc_utrancell_metrics 
ALTER COLUMN PMSUMPACKETLATENCY_000 TYPE BIGINT USING 
PMSUMPACKETLATENCY_000::BIGINT;
BEGIN
DROP VIEW
ERROR:  attribute 424 has wrong type
DETAIL:  Table has type smallint, but query expects integer.
ts=# 

ts=# begin; drop view umts_eric_ch_switch_view, eric_umts_rnc_utrancell_view, 
umts_eric_cell_integrity_view; ALTER TABLE eric_umts_rnc_utrancell_metrics 
ALTER COLUMN PMSUMPACKETLATENCY_000 TYPE BIGINT ;
BEGIN
DROP VIEW
ALTER TABLE
ts=# 

Is it useful to send something from pg_attribute, or other clues ??


-- 
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 .. ALTER COLUMN .. ERROR: attribute .. has wrong type

[Justin Pryzby]

I originally sent to psql-general some months ago, but it appears it was never
delivered (perhaps I wasn't properly subscribed?).

Failed to alter table eric_umts_rnc_utrancell_metrics: ERROR:  attribute 361 
has wrong type
DETAIL:  Table has type integer, but query expects smallint.

We've seen this at least 4 times now, on PG95 and 9.6; 3 of those times are for
the above table.

Any ideas what I can do to either reproduce it or otherwise avoid it ?

On Wed, Oct 12, 2016 at 10:25:05AM -0500, Justin Pryzby wrote:
> We've seen this happen at least once on a 9.5 server, and twice on (the same)
> server since its upgrade last week to 9.6:
> 
> > ALTER TABLE t ALTER column TYPE says: "ERROR:  attribute 81 has wrong type".
> 
> Just now under 9.6
> DETAIL: Table has type integer, but query expects smallint
> ...
> ts=# SELECT attnum, atttypid, attrelid::regclass FROM pg_attribute WHERE 
> attname='pmnopagingattemptutranrejected' ORDER BY 1 DESC,2,3;
>  attnum | atttypid |attrelid 
> +--+-
> 193 |   21 | eric_umts_rnc_utrancell_metrics
> 193 |   21 | eric_umts_rnc_utrancell_201508
> 179 |   21 | eric_umts_rnc_utrancell_201509
> 179 |   21 | eric_umts_rnc_utrancell_201510
> 179 |   21 | eric_umts_rnc_utrancell_201511
> 179 |   21 | eric_umts_rnc_utrancell_201602
> [...]
> 179 |   21 | eric_umts_rnc_utrancell_201610
> 179 |   21 | eric_umts_rnc_utrancell_201611
> (17 rows)
> 
> Last week (same server, same table, still 9.6):
> DETAIL: Table has type real, but query expects smallint
> 
> In July (different server) under 9.5
> DETAIL: Table has type real, but query expects smallint
> ...
> SELECT atttypid, attnum, attrelid::regclass FROM pg_attribute WHERE 
> attname='c_84150886'
>  atttypid | attnum |  attrelid   
> --++-
>21 |200 | huawei_msc_trunkgrp_201605
>21 |200 | huawei_msc_trunkgrp_201604
>21 |200 | huawei_msc_trunkgrp_201603
>21 |200 | huawei_msc_trunkgrp_201602
>21 |200 | huawei_msc_trunkgrp_201512
>21 |200 | huawei_msc_trunkgrp_201511
>21 |200 | huawei_msc_trunkgrp_201510
>21 |200 | huawei_msc_trunkgrp_201508
>21 |200 | huawei_msc_trunkgrp_201507
>21 |200 | huawei_msc_trunkgrp_201506
>21 |200 | huawei_msc_trunkgrp_201505
>21 |200 | huawei_msc_trunkgrp_201607
>21 |200 | huawei_msc_trunkgrp_201606
>21 |200 | huawei_msc_trunkgrp_201608
>21 |201 | huawei_msc_trunkgrp_metrics
>21 |200 | huawei_msc_trunkgrp_201509
>21 |200 | huawei_msc_trunkgrp_201601
> (17 rows)
> 
> I don't have a clear recollection how I solved this in July; possibly by
> restoring the (historic, partition) table from backup.
> 
> Last week again again just now (both under 9.6), a colleague found that he was
> able to avoid the error by ALTER TYPE without USING.
> 
> Note that we ALTER TABLE .. NO INHERIT the partitions for all but the most
> recent 2 months before ALTERing them (or the parent).  The "ALTER NO INHERIT"
> and the ALTER TYPE of historic partitions are done outside of a transaction in
> order to avoid large additional disk use otherwise used when ALTERing a parent
> with many or large children (the sum of the size of the children).


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

[Joe Conway]

On 01/02/2017 10:55 AM, Joe Conway wrote:
> On the 9.2 and 9.3 branches I see two warnings:

> This one once:
> ---
> plancache.c:1197:9: warning: ‘plan’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
> 
> And this one once per bison file:
> ---
> gram.y:169.1-13: warning: deprecated directive, use ‘%name-prefix’
> [-Wdeprecated]
>  %name-prefix="base_yy"
>  ^

> Starting in 9.5 I only get the plancache.c warning and this one:
> ---
> lwlock.c:1555:5: warning: ‘mode’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
>   if (mode == LW_EXCLUSIVE)

Peter Eisentraut's Bison deprecation warnings patch (per Tom's reply
nearby in this thread) back-patched to 9.2 and 9.3 branches.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=55fb759ab3e7543a6be72a35e6b6961455c5b393

Stephen Frosts's plancache.c back-patched to 9.6 through 9.2
and lwlock.c back-patched 9.6 through 9.5:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=d97b14ddab2059e1d73c0cd17f26bac4ef13e682

> For the sake of completeness, in 9.4. I get the plancache.c warning and
> this one:
> ---
> basebackup.c:1284:6: warning: variable ‘wait_result’ set but not used
> [-Wunused-but-set-variable]
>   int   wait_result;

This one is no longer seen -- I must have neglected to pull before
making that comment.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Shrink volume of default make output

[Tom Lane]

Jim Nasby  writes:
> The recent thread about compiler warnings got me thinking about how it's 
> essentially impossible to notice warnings with default make output. 
> Perhaps everyone just uses make -s by default, though that's a bit 
> annoying since you get no output unless something does warn (and then 
> you don't know what directory it was in).

> Is it worth looking into this? I'm guessing this may be moot with the 
> CMake work, but it's not clear when that'll make it in. In the meantime, 
> ISTM http://stackoverflow.com/a/218295 should be an easy change to make 
> (though perhaps with a variable that gives you the old behavior).

I'm not really sure which of the kluges in that article you're proposing
we adopt, but none of them look better than "make -s" to me.  Also,
none of them would do anything about make's own verbosity such as
"entering/leaving directory" lines.

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

[HACKERS] Shrink volume of default make output

[Jim Nasby]

The recent thread about compiler warnings got me thinking about how it's 
essentially impossible to notice warnings with default make output. 
Perhaps everyone just uses make -s by default, though that's a bit 
annoying since you get no output unless something does warn (and then 
you don't know what directory it was in).


Is it worth looking into this? I'm guessing this may be moot with the 
CMake work, but it's not clear when that'll make it in. In the meantime, 
ISTM http://stackoverflow.com/a/218295 should be an easy change to make 
(though perhaps with a variable that gives you the old behavior).

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] increasing the default WAL segment size

[Jim Nasby]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

General comments:
There was some discussion about the impact of this on small installs, 
particularly around min_wal_size. The concern was that changing the default 
segment size to 64MB would significantly increase min_wal_size in terms of 
bytes. The default value for min_wal_size is 5 segments, so 16MB->64MB would 
mean going from 80MB to 320MB. IMHO if you're worried about that then just 
initdb with a smaller segment size. There's probably a number of other changes 
a small environment wants to make besides that. Perhaps it'd be worth making 
DEFAULT_XLOG_SEG_SIZE a configure option to better support that.

It's not clear from the thread that there is consensus that this feature is 
desired. In particular, the performance aspects of changing segment size from a 
C constant to a variable are in question. Someone with access to large hardware 
should test that. Andres[1] and Robert[2] did suggest that the option could be 
changed to a bitshift, which IMHO would also solve some sanity-checking issues.

+* initdb passes the WAL segment size in an environment variable. We 
don't
+* bother doing any sanity checking, we already check in initdb that the
+* user gives a sane value.

That doesn't seem like a good idea to me. If anything, the backend should 
sanity-check and initdb just rely on that. Perhaps this is how other initdb 
options work, but it still seems bogus. In particular, verifying the size is a 
power of 2 seems important, as failing that would probably be ReallyBad(tm).

The patch also blindly trusts the value read from the control file; I'm not 
sure if that's standard procedure or not, but ISTM it'd be worth 
sanity-checking that as well.

The patch leaves the base GUC units for min_wal_size and max_wal_size as the # 
of segments. I'm not sure if that's a great idea.

+ * convert_unit
+ *
+ * This takes the value in kbytes and then returns value in user-readable 
format

This function needs a more specific name, such as pretty_print_kb().

+   /* Check if wal_segment_size is in the power of 2 */
+   for (i = 0;; i++, pow2 = pow(2, i))
+   if (pow2 >= wal_segment_size)
+   break;
+
+   if (wal_segment_size != 1 && pow2 > wal_segment_size)
+   {
+   fprintf(stderr, _("%s: WAL segment size must be in the 
power of 2\n"), progname);
+   exit(1);
+   }

IMHO it'd be better to use the n & (n-1) check detailed at [3].

Actually, there's got to be other places that need to check this, so it'd be 
nice to just create a function that verifies a number is a power of 2.

+   if (log_fname != NULL)
+   XLogFromFileName(log_fname, , );
+

Please add a comment about why XLogFromFileName has to be delayed.

 /*
+ * DEFAULT_XLOG_SEG_SIZE is the size of a single WAL file.  This must be a 
power
+ * of 2 and larger than XLOG_BLCKSZ (preferably, a great deal larger than
+ * XLOG_BLCKSZ).
+ *
+ * Changing DEFAULT_XLOG_SEG_SIZE requires an initdb.
+ */
+#define DEFAULT_XLOG_SEG_SIZE  (16*1024*1024)

That comment isn't really accurate. It would be more useful to explain that 
DEFAULT_XLOG_SEG_SIZE is the default size of a WAL segment used by initdb if a 
different value isn't specified.

1: 
https://www.postgresql.org/message-id/20161220082847.7t3t6utvxb6m5tfe%40alap3.anarazel.de
2: 
https://www.postgresql.org/message-id/CA%2BTgmoZTgnL25o68uPBTS6BD37ojD-1y-N88PkP57FzKqwcmmQ%40mail.gmail.com
3: 
http://stackoverflow.com/questions/108318/whats-the-simplest-way-to-test-whether-a-number-is-a-power-of-2-in-c
-- 
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] Broken atomics code on PPC with FreeBSD 10.3

[Tom Lane]

I wrote:
> But that doesn't really detract from my point, which is that it's
> totally silly for us to imagine that char and word-wide atomic ops are
> interchangeable on all platforms and we can flip a coin to decide which
> to use as long as the configure probes don't fail outright.  Even given
> working code for the byte case, we ought to prefer int atomics on PPC.
> On other platforms, maybe the preference goes the other way.  I'd be
> inclined to follow the hard-won knowledge embedded in s_lock.h about
> which to prefer.

After further study, I'm inclined to just propose that we flip the default
width of pg_atomic_flag in generic-gcc.h: use int not char if both are
available.  The only modern platform where that's the wrong thing is x86,
and that case is irrelevant here because we'll be using arch-x86.h not
generic-gcc.h.

A survey of s_lock.h shows that we prefer char-width slock_t only on
these architectures:

x86
sparc (but not sparcv9, there we use int)
m68k
vax

So basically, the existing coding is optimizing for obsolete hardware,
and not even very much of that.

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] Cluster wide option to control symbol case folding

[Greg Stark]

On 25 December 2016 at 09:40, Lewis, Ian (Microstar Laboratories)
 wrote:
> So, the current behavior already breaks many tools unless one accepts
> that all symbols on the server are lower case. At root, based on reading
> the threads you provided, this probably indicates defects in the tools,
> rather than a problem with PostgreSQL. My reading of the standard text
> quoted in various places is that any mixed case identifier returned from
> the catalog has to be quoted to match in a query (whether you fold to
> lower or upper case).

Well tools that work with user-defined columns and make assumptions
that they don't require quoting are just buggy.

But the problem with configurable quoting rules is a bit different.
Imagine your application later decides to depend on PostGIS. So you
load the PostGIS extension and perhaps also some useful functions you
found on Stack Overflow for solving some GIS problem you have. Those
extensions will create objects and then work with those objects and
may use CamelCase for clarity -- in fact I think PostGIS functions are
documented as CamelCase.  The PostGIS extensions might not work on
your system with different case rules if they haven't been 100%
consistent with their camelCasing, and the functions from
StackOverflow would be even less likely to work.

If there was some way to scope this setting lexically so it only
affected code that's defined in specific place that might be safer.
But I don't think things are currently organized that way. If you're
only concerned with server-side functions it wouldn't be hard to have
a specific pl language that was case sensitive though it might be
tricky to do to pl/pgsql due to the way pl/pgsql depends on the sql
parser.

-- 
greg


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

[Tom Lane]

Joe Conway  writes:
> On 01/02/2017 11:09 AM, Tom Lane wrote:
>> The bison issue is discussed in
>> https://www.postgresql.org/message-id/flat/E1WpjkB-0003zA-N4%40gemulon.postgresql.org

> Ah, thanks. I vaguely remember that thread now.

> Looks like there was some consensus for applying Peter's patch with the
> addition of a comment, but apparently that never happened. Would we
> still consider that for 9.2 and 9.3 branches?

Sure it did, see 55fb759ab3e7543a6be72a35e6b6961455c5b393.
That's why you don't see the complaints in 9.4 and up.
I'm not sure why Peter didn't back-patch it, but doing so now seems
safe enough.

> Any thoughts on fixing the other warnings?

I'm okay with small, low-risk patches to silence warnings in back
branches.  Like Robert, I'd be concerned about anything invasive.

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] merging some features from plpgsql2 project

[Pavel Stehule]

2017-01-02 20:16 GMT+01:00 Jim Nasby :

> On 1/2/17 12:06 PM, Pavel Stehule wrote:
>
>> SELECT (polymorphiccomposite).* INTO c1, c2; -- take first two columns
>>
>> SELECT xx FROM tab ORDER BY yy INTO target -- more rows not a issue
>>
>> I understand plpgsql_extra_errors as feature that can be enabled on
>> developer, test, or preprod environments and can help to identify some
>> strange places.
>>
>
> Yes, but the two cases you mentioned above are the "strange" cases, and
> you should have to do something extra to allow those, not the other way
> around.


The second example is really strange. But the first example is used in
composite types conversion - when you convert from base to extend type.
This routine is used in plpgsql when you use a assignment statement

composite_var := another_composite_var


>
> I think instead of tying these to extra_*, each GUC should accept a
>> LOG level.
>>
>>
>> Why? Why the none, warning, error are not enough? Why are you think so
>> separate GUC can be better than plpgsql_extra_* ?
>>
>> The fast setting plpgsql.extra_errors = 'all' can switch to some "safe"
>> configuration.
>> The fast setting plpgsql.extra_warnings = 'all' can helps with
>> identification, but doesn't break production (or doesn't breaks other
>> tests)
>>
>
> I see two problems with those settings:
>
> 1) Neither is enabled by default, so 90% of users have no idea they exist.
> Obviously that's an easy enough fix, but...
>

We can strongly talk about it - there can be a chapter in plpgsql doc. Now,
the patterns and antipatterns are not officially documented.



> 2) There's no way to incrementally change those values for a single
> function. If you've set extra_errors = 'all' globally, a single function
> can't say "turn off the too many rows setting for this function".
>

We can enhance the GUC syntax like "all -too_many_rows,-xxx"


>
> BTW, while I can see value in being able to change these settings for an
> entire function, I think the recommended use should be to only change them
> for a specific statement.


What you can do in plain assign statement

target := expression ?

My border is any compatibility break - and I would not to across it. First
issue is probably harder

related to typo "select 1 x into c1,c2" and it can be detected by
plpgsql_check.

Second issue is not a performance issue today (we read only 2 rows
everytime) and it is hard how often it returns wrong result. This issue
cannot be detected by plpgsql_check now.

Regards

Pavel

>
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)
>

Re: [HACKERS] Compiler warnings

[Joe Conway]

On 01/02/2017 11:09 AM, Tom Lane wrote:
> Joe Conway  writes:
>> If there is agreement on fixing these warnings, other than the bison
>> generated warning, I would be happy to do it. I'd also be happy to look
>> for a fix the bison warning as well if desired, but that should be
>> handled separately I would think.
> 
> The bison issue is discussed in
> https://www.postgresql.org/message-id/flat/E1WpjkB-0003zA-N4%40gemulon.postgresql.org

Ah, thanks. I vaguely remember that thread now.

Looks like there was some consensus for applying Peter's patch with the
addition of a comment, but apparently that never happened. Would we
still consider that for 9.2 and 9.3 branches?

Any thoughts on fixing the other warnings?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Cluster wide option to control symbol case folding

[Jim Nasby]

On 1/2/17 12:25 PM, Robert Haas wrote:

But, I can easily imagine a good number of people deciding they want
mixed case on the server, and so quoting their identifiers. And, then
deciding PostgreSQL is defective, rather than deciding their favorite
administration or query tool is defective. Almost all of the tools I
tried worked fine when I had all lower case symbols on the server. Based
on observing the generated SQL, most of the tools that failed for me
when I had mixed case symbols on the server would work against a case
preserving mode in PostgreSQL. The tools generally pass through the
catalog reported symbols without manipulation.

Right.  Tom's argument makes a lot of sense when you think about this
from the point of view of someone writing extensions or tools that are
designed to work with anybody's PostgreSQL instance.  When you think
about it from the point of view of a user wanting to write an
application that can work with any of a number of databases, then your
argument has a lot of merit to it.  I'm not sure there's any way to
split the baby here: tool authors will obviously prefer that
PostgreSQL's behavior in this area be invariable, while people trying
to develop portable database applications will prefer configurability.
As far as I can see, this is a zero sum game that is bound to have one
winner and one loser.


I do wonder what the authors of those tools are using to test them. My 
guess is they're just hitting the default postgres database, which has 
no quotable identifiers.


I'd find it useful to have a contrib module/tool that would create a 
full-blown test database that contains objects of every possible type, 
including using identifiers that need quotes. It'd also be useful to 
test leading and trailing whitespace.


Having this would be useful for seeing what some of the more unusual 
objects look like in the catalog, and would simplify tests that I create 
for my extensions somewhat. If tool authors knew it existed they could 
use it for testing. It'd certainly be useful for testing things like 
pg_dump and event triggers.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Tom Lane]

Jim Nasby  writes:
> On 1/2/17 11:39 AM, David Steele wrote:
>> On 1/2/17 12:30 PM, Jim Nasby wrote:
>>> On 1/1/17 9:48 AM, Peter Eisentraut wrote:
>>> Perhaps we should split the difference and do what we did for XML:
>>> provide a contrib module with alias functions using the old (xlog) names.

>> -1
>> Since these functions are normally used by admins and not generally used
>> in SQL and functions, I'm not convinced the maintenance of the extension
>> would be worth it.  Admins are free to create whatever aliases they need
>> to get their work done.

> BTW, I think fears of the maintenance cost of a contrib module are 
> pretty overblown... it's not like we change these functions that often. 
> We have added quite a few in the last few releases, but we don't need 
> backwards compatibility for new stuff.

I'm also -1 on this idea.  If we're going to provide backwards
compatibility, we should just leave the old names in the core.
Providing an extension is more work for *everybody* --- for us, and
for the users who will have to learn about and install the extension.
I don't see any gain to justify that work, either.

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] merging some features from plpgsql2 project

[Jim Nasby]

On 1/2/17 12:06 PM, Pavel Stehule wrote:

SELECT (polymorphiccomposite).* INTO c1, c2; -- take first two columns

SELECT xx FROM tab ORDER BY yy INTO target -- more rows not a issue

I understand plpgsql_extra_errors as feature that can be enabled on
developer, test, or preprod environments and can help to identify some
strange places.


Yes, but the two cases you mentioned above are the "strange" cases, and 
you should have to do something extra to allow those, not the other way 
around.



I think instead of tying these to extra_*, each GUC should accept a
LOG level.


Why? Why the none, warning, error are not enough? Why are you think so
separate GUC can be better than plpgsql_extra_* ?

The fast setting plpgsql.extra_errors = 'all' can switch to some "safe"
configuration.
The fast setting plpgsql.extra_warnings = 'all' can helps with
identification, but doesn't break production (or doesn't breaks other tests)


I see two problems with those settings:

1) Neither is enabled by default, so 90% of users have no idea they 
exist. Obviously that's an easy enough fix, but...
2) There's no way to incrementally change those values for a single 
function. If you've set extra_errors = 'all' globally, a single function 
can't say "turn off the too many rows setting for this function".


BTW, while I can see value in being able to change these settings for an 
entire function, I think the recommended use should be to only change 
them for a specific statement.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Jim Nasby]

On 1/2/17 11:39 AM, David Steele wrote:



On 1/2/17 12:30 PM, Jim Nasby wrote:

On 1/1/17 9:48 AM, Peter Eisentraut wrote:

On 12/30/16 9:57 AM, Stephen Frost wrote:

Additionally, people who are actually using these bits of the system are
almost certainly going to have to adjust things for the directory
change,


Some *xlog* functions are commonly used to measure replay lag.  That
usage would not be affected by the directory renaming.  Renaming those
functions would only serve to annoy users and have them delay their
upgrades.


Perhaps we should split the difference and do what we did for XML:
provide a contrib module with alias functions using the old (xlog) names.


-1

Since these functions are normally used by admins and not generally used
in SQL and functions, I'm not convinced the maintenance of the extension
would be worth it.  Admins are free to create whatever aliases they need
to get their work done.


AIUI several others are arguing that this name change is going to break 
a lot of user monitoring code. I certainly agree with Stephen that some 
of the *xlog* functions are used for monitoring replay lag. So I think a 
backwards compatibility fix is reasonable.


Why would we force users to each come up with their own solution to this 
when we can just provide one?


BTW, I think fears of the maintenance cost of a contrib module are 
pretty overblown... it's not like we change these functions that often. 
We have added quite a few in the last few releases, but we don't need 
backwards compatibility for new stuff.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


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

[Tom Lane]

Joe Conway  writes:
> If there is agreement on fixing these warnings, other than the bison
> generated warning, I would be happy to do it. I'd also be happy to look
> for a fix the bison warning as well if desired, but that should be
> handled separately I would think.

The bison issue is discussed in
https://www.postgresql.org/message-id/flat/E1WpjkB-0003zA-N4%40gemulon.postgresql.org

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] Make pg_basebackup -x stream the default

[Simon Riggs]

On 31 December 2016 at 23:47, Michael Paquier  wrote:

> Other than that the patch looks good to me. Tests pass.

+1

-- 
Simon Riggshttp://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] Compiler warnings

[Joe Conway]

On 01/02/2017 10:18 AM, Robert Haas wrote:
> On Thu, Dec 29, 2016 at 10:47 AM, Joe Conway  wrote:
>> Shouldn't this be back-patched? The plancache warning goes back through
>> 9.2 (at least) and the lwlocks warning through 9.5 (or maybe it was 9.4).
> 
> Warnings are going to be different for each individual developer, but
> I am cautiously in favor making more of an effort to fix back-branch
> warnings provided that it doesn't generate too much code churn.  For
> example, if your toolchain generates only these two warnings on 9.2
> then, sure, let's back-port these two fixes; making things
> warning-clean is great.  But if there are dozens or hundreds of
> warnings currently, fixing only a handful of those warnings probably
> isn't valuable, and fixing all of them is probably a little more risk
> than we necessarily want to take.  Someone could goof and make a bug.
> On my MacBook Pro with my toolchain, we're warning-clean back to 9.3
> or 9.4, and before that there are some problems -- most annoyingly the
> fact that 73b416b2e41237b657d29d8f42a4bb34bf700928 couldn't be easily
> backported to older branches.  I don't think it would be crazy to try
> to get all of the warnings I see fixed up and it would be convenient
> for me, but I haven't been willing to do the work, either.
> 

FWIW, I'm running Mint 18 which is based on Unbuntu 16.04 I believe. On
the 9.2 and 9.3 branches I see two warnings:

This one once:
---
plancache.c:1197:9: warning: ‘plan’ may be used uninitialized in this
function [-Wmaybe-uninitialized]

And this one once per bison file:
---
gram.y:169.1-13: warning: deprecated directive, use ‘%name-prefix’
[-Wdeprecated]
 %name-prefix="base_yy"
 ^

The plancache.c fix in Stephen's patch was really simple: initialize
plan = NULL and add an assert. To me that seems like something we should
definitely back-patch.

On the other hand, seems like we have had bison related warnings of one
kind or another since I first got involved with Postgres. So while it
would be nice to make that one go away, it somehow bothers me less. It
also goes away starting in 9.4 anyway.


Starting in 9.5 I only get the plancache.c warning and this one:
---
lwlock.c:1555:5: warning: ‘mode’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
  if (mode == LW_EXCLUSIVE)
 ^

That is the other warning fixed in Stephens commit to master.

For the sake of completeness, in 9.4. I get the plancache.c warning and
this one:
---
basebackup.c:1284:6: warning: variable ‘wait_result’ set but not used
[-Wunused-but-set-variable]
  int   wait_result;
  ^

Seems like that should be easily fixed.

If there is agreement on fixing these warnings, other than the bison
generated warning, I would be happy to do it. I'd also be happy to look
for a fix the bison warning as well if desired, but that should be
handled separately I would think.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Cluster wide option to control symbol case folding

[Robert Haas]

On Sun, Dec 25, 2016 at 4:40 AM, Lewis, Ian (Microstar Laboratories)
 wrote:
> I assume you are talking about general purpose tools that attempt to
> interact with any database in any configuration. Obviously, a purpose
> built tool, such as our own internal database applications, would be
> designed only for the behavior of the databases it is intended to work
> against.

Right.

> So, the current behavior already breaks many tools unless one accepts
> that all symbols on the server are lower case. At root, based on reading
> the threads you provided, this probably indicates defects in the tools,
> rather than a problem with PostgreSQL. My reading of the standard text
> quoted in various places is that any mixed case identifier returned from
> the catalog has to be quoted to match in a query (whether you fold to
> lower or upper case).
>
> But, I can easily imagine a good number of people deciding they want
> mixed case on the server, and so quoting their identifiers. And, then
> deciding PostgreSQL is defective, rather than deciding their favorite
> administration or query tool is defective. Almost all of the tools I
> tried worked fine when I had all lower case symbols on the server. Based
> on observing the generated SQL, most of the tools that failed for me
> when I had mixed case symbols on the server would work against a case
> preserving mode in PostgreSQL. The tools generally pass through the
> catalog reported symbols without manipulation.

Right.  Tom's argument makes a lot of sense when you think about this
from the point of view of someone writing extensions or tools that are
designed to work with anybody's PostgreSQL instance.  When you think
about it from the point of view of a user wanting to write an
application that can work with any of a number of databases, then your
argument has a lot of merit to it.  I'm not sure there's any way to
split the baby here: tool authors will obviously prefer that
PostgreSQL's behavior in this area be invariable, while people trying
to develop portable database applications will prefer configurability.
As far as I can see, this is a zero sum game that is bound to have one
winner and one loser.

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

[Robert Haas]

On Thu, Dec 29, 2016 at 10:47 AM, Joe Conway  wrote:
> Shouldn't this be back-patched? The plancache warning goes back through
> 9.2 (at least) and the lwlocks warning through 9.5 (or maybe it was 9.4).

Warnings are going to be different for each individual developer, but
I am cautiously in favor making more of an effort to fix back-branch
warnings provided that it doesn't generate too much code churn.  For
example, if your toolchain generates only these two warnings on 9.2
then, sure, let's back-port these two fixes; making things
warning-clean is great.  But if there are dozens or hundreds of
warnings currently, fixing only a handful of those warnings probably
isn't valuable, and fixing all of them is probably a little more risk
than we necessarily want to take.  Someone could goof and make a bug.
On my MacBook Pro with my toolchain, we're warning-clean back to 9.3
or 9.4, and before that there are some problems -- most annoyingly the
fact that 73b416b2e41237b657d29d8f42a4bb34bf700928 couldn't be easily
backported to older branches.  I don't think it would be crazy to try
to get all of the warnings I see fixed up and it would be convenient
for me, but I haven't been willing to do the work, either.

-- 
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] Clarifying "server starting" messaging in pg_ctl start without --wait

[Ryan Murphy]

> Making --wait the default may or may not be sensible -- I'm not sure
> -- but removing --no-wait is clearly a bad idea, and we shouldn't do
> it.  The fact that the problems created by removing it might be
> solvable doesn't mean that it's a good idea to create them in the
> first place.
>
>
>
I agree with Robert - pg_ctl is no doubt used in all kinds of scripts that
would then have to change.
It may make sense to have --wait be the default though - certainly less
confusing to new users!

Re: [HACKERS] Clarifying "server starting" messaging in pg_ctl start without --wait

[Robert Haas]

On Fri, Dec 23, 2016 at 7:25 PM, Jim Nasby  wrote:
> On 12/23/16 6:10 PM, Tom Lane wrote:
>> Michael Paquier  writes:
>>> Is there still a use case for --no-wait in the real world?
>>
>> Sure.  Most system startup scripts aren't going to want to wait.
>> If we take it out those people will go back to starting the postmaster
>> by hand.
>
> Presumably they could just background it... since it's not going to be
> long-lived it's presumably not that big a deal. Though, seems like many
> startup scripts like to make sure what they're starting is actually working.

Making --wait the default may or may not be sensible -- I'm not sure
-- but removing --no-wait is clearly a bad idea, and we shouldn't do
it.  The fact that the problems created by removing it might be
solvable doesn't mean that it's a good idea to create them in the
first place.

-- 
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] merging some features from plpgsql2 project

[Pavel Stehule]

2017-01-02 18:36 GMT+01:00 Jim Nasby :

> On 1/1/17 12:17 PM, Pavel Stehule wrote:
>
>> I wrote some initial patch
>>
>> Do you think so has sense to continue in this topic?
>>
>
> Perhaps I'm not understanding what plpgsql_extra_errors does, but I don't
> think either of these should depend on that being true. IMO these two
> checks should be default to throwing an exception.
>

There are use cases where these patters should be used and has sense like

SELECT (polymorphiccomposite).* INTO c1, c2; -- take first two columns

SELECT xx FROM tab ORDER BY yy INTO target -- more rows not a issue

I understand plpgsql_extra_errors as feature that can be enabled on
developer, test, or preprod environments and can help to identify some
strange places.


>
> I think instead of tying these to extra_*, each GUC should accept a LOG
> level.


Why? Why the none, warning, error are not enough? Why are you think so
separate GUC can be better than plpgsql_extra_* ?

The fast setting plpgsql.extra_errors = 'all' can switch to some "safe"
configuration.
The fast setting plpgsql.extra_warnings = 'all' can helps with
identification, but doesn't break production (or doesn't breaks other tests)

Regards

Pavel


>
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)
>

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[David Steele]

On 1/2/17 12:30 PM, Jim Nasby wrote:
> On 1/1/17 9:48 AM, Peter Eisentraut wrote:
>> On 12/30/16 9:57 AM, Stephen Frost wrote:
>>> Additionally, people who are actually using these bits of the system are
>>> almost certainly going to have to adjust things for the directory
>>> change,
>>
>> Some *xlog* functions are commonly used to measure replay lag.  That
>> usage would not be affected by the directory renaming.  Renaming those
>> functions would only serve to annoy users and have them delay their
>> upgrades.
> 
> Perhaps we should split the difference and do what we did for XML:
> provide a contrib module with alias functions using the old (xlog) names.

-1

Since these functions are normally used by admins and not generally used
in SQL and functions, I'm not convinced the maintenance of the extension
would be worth it.  Admins are free to create whatever aliases they need
to get their work done.

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


-- 
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] merging some features from plpgsql2 project

[Jim Nasby]

On 1/1/17 12:17 PM, Pavel Stehule wrote:

I wrote some initial patch

Do you think so has sense to continue in this topic?


Perhaps I'm not understanding what plpgsql_extra_errors does, but I 
don't think either of these should depend on that being true. IMO these 
two checks should be default to throwing an exception.


I think instead of tying these to extra_*, each GUC should accept a LOG 
level.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Jim Nasby]

On 1/1/17 9:48 AM, Peter Eisentraut wrote:

On 12/30/16 9:57 AM, Stephen Frost wrote:

Additionally, people who are actually using these bits of the system are
almost certainly going to have to adjust things for the directory
change,


Some *xlog* functions are commonly used to measure replay lag.  That
usage would not be affected by the directory renaming.  Renaming those
functions would only serve to annoy users and have them delay their
upgrades.


Perhaps we should split the difference and do what we did for XML: 
provide a contrib module with alias functions using the old (xlog) names.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Fixing pgbench's logging of transaction timestamps

[Tom Lane]

I wrote:
> BTW, why is it that the --aggregate-interval option is unsupported on
> Windows?  Is that an artifact of the same disease of assuming too much
> about how instr_time is represented?  I don't see any very good reason
> for it other than the weird decision to store the result of
> INSTR_TIME_GET_DOUBLE in a "long", which seems rather broken in any case.

After looking closer, I see the real issue is that it prints the integer
part of INSTR_TIME_GET_DOUBLE and documents that as being a Unix
timestamp.  So that's not going to do either.  I solved it the same way
as in the other code path, ie just eat the cost of doing our own time
inquiry.

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] Odd behavior with PG_TRY

[Jim Nasby]

On 1/2/17 1:31 AM, Amit Kapila wrote:

On Mon, Jan 2, 2017 at 11:14 AM, Jim Nasby  wrote:

In the attached patch (snippet below), I'm seeing something strange with
args->in.r.atts[]. Prior to entering the PG_TRY block, I can inspect things
in lldb just fine:

(lldb) call args->in.r.atts[0].func
(PLyDatumToObFunc) $49 = 0x00010fc4dc70
(plpython3.so`PLyString_FromDatum at plpy_typeio.c:621)
(lldb) call >in.r.atts[0]
(PLyDatumToOb *) $50 = 0x7fd2b302f6d0
(lldb) call args->in.r.atts[0]
(PLyDatumToOb) $51 = {
  func = 0x00010fc4dc70 (plpython3.so`PLyString_FromDatum at
plpy_typeio.c:621)
  typfunc = {
fn_addr = 0x00010f478b50 (postgres`textout at varlena.c:521)
fn_oid = 47
...

But I'm getting a EXC_BAD_ACCESS (code=1, address=0xb302f6d0) on the last if
in the snippet below. Looking at the variables again, I see:

(lldb) call args->in.r.atts[i].func
error: Couldn't apply expression side effects : Couldn't dematerialize a
result variable: couldn't read its memory
(lldb) call args->in.r.atts[i]
error: Couldn't apply expression side effects : Couldn't dematerialize a
result variable: couldn't read its memory



Looks strange, what is the value of 'i'?  Did you get the same result
if you try to print args->in.r.atts[0] inside PG_TRY?


i is 0, makes no difference:

(lldb) call i
(int) $56 = 0
(lldb) call args->in.r.atts[0]
error: Couldn't apply expression side effects : Couldn't dematerialize a 
result variable: couldn't read its memory

(lldb)
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] proposal: session server side variables

[Fabien COELHO]

Hello,

In my proposal was support for transaction scope - ON COMMIT RESET 
clause should be ok


Could you update the wiki, both the proposal and the use-case
implementation, to reflect this point?

Moreover, is there any actual use-case for non-transactional secure
half-persistent session variables? AFAICS the "secure" part implies both
permissions and transactional for the presented security-related use case.
If there is no use case for these combined features, then ISTM that you
should update to proposal so that the variables are always transactional,
which is both simpler, more consistent, and I think more acceptable.


If you are transaction sensitive, then you have to be sensitive to
subtransactions - then the work is much more complex.


Maybe, probably, I do not really know. For now, I'm trying to determine 
how the proposals fits Craig's use case.


The current status is that both proposals are useless because the use case 
needs "some" transactional property for security. But probably some 
improvements are possible.



Is there use case, when you would to play with transactions and variables
and RESET is not enough?


I do not know. If you explain more clearly what is meant by a "RESET" on a 
variable when the transaction fails, then maybe I can have an opinion. 
Currently I'm just guessing in the dark the precise intended semantics.


--
Fabien.


--
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] proposal: session server side variables

[Fabien COELHO]

Attention! rollback is significantly expensive than RESET.


I'm quite unclear about the difference... Transactional for an unshared 
only-in-memory session object is probably easy to implement, no WAL is 
needed... So I do not see the difference



you have to store previous value


This does not fully answer my question.

Maybe RESET would put NULL instead of the previous value in a rollback?

If so, I must admit that I do not see any fundamental issue with holding 
temporarily the initial value of an in-memory session variables so as to 
be able to rool it back if required...


There are no any product where variables are transactional - we should 
not to create wheel.


Well, AFAICS PostgreSQL GUCs are transactional.



that is exception ..


That is just logic: if you make an argument based on "it does not exist", 
then the argument is void if someone produces a counter example.



show me some transactiinal variables from msql, oracle, db2


I do not really know these three particular products. All I can say is 
that from a semantical point of view the contents of any one-row temporary 
relation is somehow a transactional session variable. However I do not 
know whether the 3 cited products have temporary tables, this is just a 
guess.


--
Fabien.


--
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] Replication slot xmin is not reset if HS feedback is turned off while standby is shut down

[Craig Ringer]

On 2 Jan. 2017 20:20, "Simon Riggs"  wrote:

On 21 December 2016 at 13:23, Simon Riggs  wrote:

> Fix it up and I'll commit. Thanks for the report.

I was hoping for some more effort from Ants to correct this.

I'll commit Craig's new tests for hs feedback before this, so it can
go in with a Tap test, so I imagine we're about a week away from
commit on this.


I posted a revised version of Ants's patch that passes the shell script
test.

A TAP  test would likely make sene though, I agree.

Re: [HACKERS] Replication/backup defaults

[Simon Riggs]

On 2 January 2017 at 09:39, Magnus Hagander  wrote:

> The conclusion has been that our defaults should really allow people to take
> backups of their systems, and they currently don't.
>
> Making things run faster is tuning, and people should expect to do that if
> they need things to run faster. But being able to make a backup is pretty
> fundamental.

In the hope of making things better in 10.0, I remove my objection. If
people want to use wal_level = minimal they can restart their server
and they can find that out in the release notes.

Should we set wal_level = replica or wal_level = logical as the
default for 10.0?

-- 
Simon Riggshttp://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] Logical Replication WIP

[Petr Jelinek]

On 02/01/17 05:23, Steve Singer wrote:
> On 12/30/2016 05:53 AM, Petr Jelinek wrote:
>> Hi,
>>
>> I rebased this for the changes made to inheritance and merged in the
>> fixes that I previously sent separately.
>>
>>
>>
> 
> 
> I'm not sure if the following is expected or not
> 
> I have 1 publisher and 1 subscriber.
> I then do pg_dump on my subscriber
> ./pg_dump -h localhost --port 5441 --include-subscriptions
> --no-create-subscription-slot test|./psql --port 5441 test_b
> 
> I now can't do a drop database test_b  , which is expected
> 
> but I can't drop the subscription either
> 
> 
> test_b=# drop subscription mysub;
> ERROR:  could not drop replication origin with OID 1, in use by PID 24996
> 
>  alter subscription mysub disable;
> ALTER SUBSCRIPTION
> drop subscription mysub;
> ERROR:  could not drop replication origin with OID 1, in use by PID 24996
> 
> drop subscription mysub nodrop slot;
> 
> doesn't work either.  If I first drop the working/active subscription on
> the original 'test' database it works but I can't seem to drop the
> subscription record on test_b
> 

I guess this is because replication origins are pg instance global and
we use subscription name for origin name internally. Maybe we need to
prefix/suffix it with db oid or something like that, but that's
problematic a bit as well as they both have same length limit. I guess
we could use subscription OID as replication origin name which is
somewhat less user friendly in terms of debugging but would be unique.

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

Re: [HACKERS] proposal: session server side variables

[Pavel Stehule]

2017-01-02 11:48 GMT+01:00 Fabien COELHO :

>
> Hello Pavel,
>
> In my proposal was support for transaction scope - ON COMMIT RESET clause
>> should be ok
>>
>
> Could you update the wiki, both the proposal and the use-case
> implementation, to reflect this point?
>
> Moreover, is there any actual use-case for non-transactional secure
> half-persistent session variables? AFAICS the "secure" part implies both
> permissions and transactional for the presented security-related use case.
> If there is no use case for these combined features, then ISTM that you
> should update to proposal so that the variables are always transactional,
> which is both simpler, more consistent, and I think more acceptable.
>

If you are transaction sensitive, then you have to be sensitive to
subtransactions - then the work is much more complex.

Is there use case, when you would to play with transactions and variables
and RESET is not enough?


>
> Also, you used a TEMPORARY session variable in one implementation, but
> this is not described in the proposal, I think it is worth mentioning it
> there as well.


I will fix it.

Regards

Pavel


>
>
> --
> Fabien.
>

Re: [HACKERS] Replication slot xmin is not reset if HS feedback is turned off while standby is shut down

[Simon Riggs]

On 21 December 2016 at 13:23, Simon Riggs  wrote:

> Fix it up and I'll commit. Thanks for the report.

I was hoping for some more effort from Ants to correct this.

I'll commit Craig's new tests for hs feedback before this, so it can
go in with a Tap test, so I imagine we're about a week away from
commit on this.

-- 
Simon Riggshttp://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] [PATCH] PostgresNode.pm enhancements, pg_lsn helper, and some more recovery tests

[Simon Riggs]

On 14 November 2016 at 07:01, Craig Ringer  wrote:
> Every test I write with the TAP framework for recovery seems to need
> to wait for one node to catch up to another or examine replication
> slot state. So: attached is a patch to add helper methods for these
> tasks.
>
> Also add a new pg_lsn.pm helper class to deal with PostgreSQL's
> awkward representation of LSNs in perl. Made extra fun by our use of
> Perl 5.8.8 with no new modules, so we can't rely on having 64-bit
> integers. Provides sensible LSN comparisons. I'll be using this in
> coming patches that have to make assertions about differences between
> LSNs, and I think it's sensible to have anyway. Incorporates tests for
> the class.
>
> Finally, add some coverage of physical replication slots to recovery tests.
>
> Backpatch to 9.6 desirable, since we seem to be doing that for TAP
> infrastructure.
>
> These three are related enough, and all only touch the TAP framework,
> so I've bundled them in a series.

Good to see more tests going in.

Bit confused... can't see a caller for wait_for_slot_catchup() and the
slot tests don't call it AFAICS.

Also can't see anywhere the LSN stuff is used either.

No specific problems with the code, just don't want to commit code
that isn't used anywhere, yet.

I want to commit the extra tests soon, as a stronger test for my
recovery.conf changes.

-- 
Simon Riggshttp://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] Replication/backup defaults

[Simon Riggs]

On 2 January 2017 at 09:48, Simon Riggs  wrote:

> I'm willing to assist in a project to allow changing wal_level online
> in this release. Please let's follow that path.

wal_level looks like one of the easier ones to change without a server restart

There are actions to take in either direction, up or down. My initial
thoughts on the pseudocode would be...

reset wal_level so all new transactions see that value
/* actions after setting new value */
if (old_wal_level  < new_wal_level) /* going up */
   get list of running transactions (perhaps only those using no-WAL-opt)
else /* coming down */
{
 if (old_wal_level == logical)
  disconnect logical replication and disallow logical slots
 if (new_wal_level == minimal)
  disconnect streaming replication and disallow physical slots
}
wait for a checkpoint (fast checkpoint if no other transactions actions active)
if (list)
   wait for list of running xacts to complete
wait for a checkpoint (fast checkpoint if no other transactions actions active)
XLogReportParameters()

So it looks easier to go up than down, which is good since that is the
important direction.

-- 
Simon Riggshttp://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] proposal: session server side variables

[Fabien COELHO]

Yep, the variable value must be rolled back, I think.


Attention! rollback is significantly expensive than RESET.


I'm quite unclear about the difference... Transactional for an unshared 
only-in-memory session object is probably easy to implement, no WAL is 
needed... So I do not see the difference.



There are no any product where variables are transactional - we should not
to create wheel.


Well, AFAICS PostgreSQL GUCs are transactional.

--
Fabien.


--
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] proposal: session server side variables

[Fabien COELHO]

Hello Pavel,


In my proposal was support for transaction scope - ON COMMIT RESET clause
should be ok


Could you update the wiki, both the proposal and the use-case 
implementation, to reflect this point?


Moreover, is there any actual use-case for non-transactional secure 
half-persistent session variables? AFAICS the "secure" part implies both 
permissions and transactional for the presented security-related use case. 
If there is no use case for these combined features, then ISTM that you 
should update to proposal so that the variables are always transactional, 
which is both simpler, more consistent, and I think more acceptable.


Also, you used a TEMPORARY session variable in one implementation, but 
this is not described in the proposal, I think it is worth mentioning it 
there as well.


--
Fabien.


--
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] Replication/backup defaults

[Andres Freund]

On 2017-01-02 10:31:28 +, Simon Riggs wrote:
> We must listen to feedback, not just try to blast through it.

Not agreeing with your priorities isn't "blasting through feedback".


-- 
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] Replication/backup defaults

[Simon Riggs]

On 2 January 2017 at 10:13, Andres Freund  wrote:
> On 2017-01-02 11:05:05 +0100, Magnus Hagander wrote:
>> My claim here is that a lot *fewer* people have come to expect this
>> performance optimization, than would (quite reasonably) expect that backups
>> should work on a system without taking it down for restart to reconfigure
>> it to support that.
>
> +1
>
> As evidenced by the fact that a large fraction of those optimizations
> are actually currently entirely broken. Without anybody noticing for
> years:
> http://archives.postgresql.org/message-id/20150702220524.GA9392%40svana.org

No, the optimization works, but there is a bug in it that makes it
unsafe, not the same thing as entirely broken. That clearly needs to
be fixed, but it does not prevent the performance benefit, so that
argument is invalid.

We must listen to feedback, not just try to blast through it.

-- 
Simon Riggshttp://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] Replication/backup defaults

[Andres Freund]

On 2017-01-02 11:05:05 +0100, Magnus Hagander wrote:
> My claim here is that a lot *fewer* people have come to expect this
> performance optimization, than would (quite reasonably) expect that backups
> should work on a system without taking it down for restart to reconfigure
> it to support that.

+1

As evidenced by the fact that a large fraction of those optimizations
are actually currently entirely broken. Without anybody noticing for
years:
http://archives.postgresql.org/message-id/20150702220524.GA9392%40svana.org

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] Replication/backup defaults

[Magnus Hagander]

On Mon, Jan 2, 2017 at 10:48 AM, Simon Riggs  wrote:

> On 2 January 2017 at 09:39, Magnus Hagander  wrote:
>
> > Please do submit a patch for it.
>
> The way this is supposed to go is someone submits a patch and they
> receive feedback, then act on that feedback. If I was able to get away
> with deflecting all review comments with a simple "you fix it if you
> don't like it" there would be considerably more patches with my name
> on it accepted, but probably no further forward in real terms because
> of the loose ends it creates.
>

Fair enough.

It's just that people keep saying that this is easy, and have said so for a
long time, but nobody has written a patch for it.



> In this case, simply changing the default will remove a whole class of
> performance optimization that we have educated people to expect. I'm
> sorry to point this out but removing that will cause many real changes
> for people's systems that we will not be thanked for, even though I
> understand your reasoning and wish the same goals to be achieved.
>

My claim here is that a lot *fewer* people have come to expect this
performance optimization, than would (quite reasonably) expect that backups
should work on a system without taking it down for restart to reconfigure
it to support that.

I run into that all the time. I hear complaints about that all the time. I
have not heard a single user complain about performance loss after enabling
backups.

And how many people that rely on this optimization don't do any *other*
optimization on their system *anyway*, that would cause them to require a
restart anyway? It's not like we're taking away their ability to enable the
optimization, it's just not on by default.



> I'm willing to assist in a project to allow changing wal_level online
> in this release. Please let's follow that path.
>

Sure thing, I will be happy to help test and review such a patch.

I will still argue that the *default* should be wal_level=replica though.
Because once we have such a patch, it's trivial to re-enable this
performance optimization (at the cost of backups and replication).

//Magnus

Re: [HACKERS] Replication/backup defaults

[Simon Riggs]

On 2 January 2017 at 09:39, Magnus Hagander  wrote:

> Please do submit a patch for it.

The way this is supposed to go is someone submits a patch and they
receive feedback, then act on that feedback. If I was able to get away
with deflecting all review comments with a simple "you fix it if you
don't like it" there would be considerably more patches with my name
on it accepted, but probably no further forward in real terms because
of the loose ends it creates.

In this case, simply changing the default will remove a whole class of
performance optimization that we have educated people to expect. I'm
sorry to point this out but removing that will cause many real changes
for people's systems that we will not be thanked for, even though I
understand your reasoning and wish the same goals to be achieved.

I'm willing to assist in a project to allow changing wal_level online
in this release. Please let's follow that path.

-- 
Simon Riggshttp://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] proposal: session server side variables

[Pavel Stehule]

2017-01-02 10:39 GMT+01:00 Fabien COELHO :

>
> Hello Craig,
>
> What if setup_user() succeeds as a function but the transaction it belongs
>>> to fails for some reason (eg deferred constraints, other operation related
>>> to setting user up but outside of this function fails, there is replication
>>> issue... whatever, a transaction may fail by definition)?
>>>
>>> ISTM that the security models requires that USER_IS_AUDITOR is reverted,
>>> so although it is definitely a session variable, it must be transactional
>>> (MVCC) nevertheless.
>>>
>>
>> No strong opinion here.
>>
>> IMO the simplest answer should be the main focus here: if it's session
>> level, it's session level. Not kinda-sesion-level kinda-transaction-level.
>>
>
> There is no contradiction between session level & transactions: a session
> executes transactions, fine. TEMP tables are MVCC *and* session level.
>
> I can see occasional uses for what you describe though.
>>
>
> My question is not strictly about use, it is about a key security point
> related to the presented use case, which is about security. The whole
> discussion of the thread being about somehow-secured session variables.
>
> ISTM that if the transaction setting the value fails and the secure
> variable says that all is well thus allows other operations to proceed
> believing that the credentials have been veted while in reality they have
> not, that's no good.
>
> So my understanding of this use case is that the involved session variable
> which hold the state must be transactional. Other use cases may have
> different requirements and security implications.
>
> If we landed up with an xact scope option like we have for SET LOCAL GUCs,
>>
>
> ISTM that it is a little different. The GUC local option makes the
> variable value always disappear after the xacts, whether it succeeds or
> not. The semantics needed here is that the value must disappear if the xact
> fails but not if it succeeds, which is the current behavior of GUCs with
> is_local=FALSE.
>
> the option to mark it ON COMMIT RESET or ON COMMIT SET would be useful I
>> guess. I'm not sure if it's worth the complexity.
>>
>
> My question right now is rather to determine what are the precise and hard
> requirements of the use case.
>
> I guess defaulting to rolling back variable effects on xact rollback would
>> be ok too. Just kind of limiting.
>>
>
> Yep, the variable value must be rolled back, I think.


attention! rollback is significantly expensive than RESET.

There are no any product where variables are transactional - we should not
to create wheel.

Regards

Pavel


>
>
> --
> Fabien.
>

Re: [HACKERS] proposal: session server side variables

[Fabien COELHO]

Hello Craig,

What if setup_user() succeeds as a function but the transaction it 
belongs to fails for some reason (eg deferred constraints, other 
operation related to setting user up but outside of this function 
fails, there is replication issue... whatever, a transaction may fail 
by definition)?


ISTM that the security models requires that USER_IS_AUDITOR is 
reverted, so although it is definitely a session variable, it must be 
transactional (MVCC) nevertheless.


No strong opinion here.

IMO the simplest answer should be the main focus here: if it's session
level, it's session level. Not kinda-sesion-level kinda-transaction-level.


There is no contradiction between session level & transactions: a session 
executes transactions, fine. TEMP tables are MVCC *and* session level.



I can see occasional uses for what you describe though.


My question is not strictly about use, it is about a key security point 
related to the presented use case, which is about security. The whole 
discussion of the thread being about somehow-secured session variables.


ISTM that if the transaction setting the value fails and the secure 
variable says that all is well thus allows other operations to proceed 
believing that the credentials have been veted while in reality they have 
not, that's no good.


So my understanding of this use case is that the involved session variable 
which hold the state must be transactional. Other use cases may have 
different requirements and security implications.


If we landed up with an xact scope option like we have for SET LOCAL 
GUCs,


ISTM that it is a little different. The GUC local option makes the 
variable value always disappear after the xacts, whether it succeeds or 
not. The semantics needed here is that the value must disappear if the 
xact fails but not if it succeeds, which is the current behavior of GUCs 
with is_local=FALSE.


the option to mark it ON COMMIT RESET or ON COMMIT SET would be 
useful I guess. I'm not sure if it's worth the complexity.


My question right now is rather to determine what are the precise and hard 
requirements of the use case.


I guess defaulting to rolling back variable effects on xact rollback 
would be ok too. Just kind of limiting.


Yep, the variable value must be rolled back, I think.

--
Fabien.


--
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] Replication/backup defaults

[Magnus Hagander]

On Mon, Jan 2, 2017 at 10:32 AM, Simon Riggs  wrote:

> On 2 January 2017 at 09:21, Magnus Hagander  wrote:
> >
> >
> > On Mon, Jan 2, 2017 at 10:17 AM, Simon Riggs 
> wrote:
> >>
> >> On 31 December 2016 at 15:00, Magnus Hagander 
> wrote:
> >> > Cycling back to this topic again, but this time at the beginning of a
> >> > CF.
> >> >
> >> > Here's an actual patch to change:
> >> >
> >> >
> >> > max_wal_senders=10
> >> > max_replication_slots=20
> >>
> >> +1
> >>
> >> If that doesn't fly, it seems easy enough to introduce a
> >> "min_reserve_limit" GUC that defaults to 10 that gives a lower bound
> >> on the amount of memory we reserve for many of those shmem allocs;
> >> that can be set to 0 for people that want the old behaviour. Then we
> >> can allow changes up to the memory limit without a restart.
> >>
> >> > wal_level=replica
> >>
> >> This is more problematic because it changes behaviours.
> >
> >
> > You can't actually change the other two without changing wal_level.
>
> You could, but we currently disallow it.
>

I always assumed we disallowed it because we would have to write actual
code to make it safe.



> >> A more useful approach would be to bring all the things needed to
> >> enable replication into one ALTER SYSTEM command, so people have just
> >> one thing they need to execute and it will work out the details and
> >> locations for you.
> >> That way we can maintain the defaults yet make it easier to enable in
> >> a useful way.
> >
> >
> > Sure, that would be great - the core being the ability to change these
> > things without a restart. But I would argue for not letting perfection
> get
> > in the way of progress, and do this anyway. I doubt there is any way the
> > bigger change is going to get done for 10 at this point, so we should
> give
> > people the ability to do backups off a default installation already.
>
> We could fairly easily change wal_level without restart; its been
> discussed for many years.
>
> The problem from my perspective is that you're immediately turning off
> the performance benefits for initial bulk loads.
>

We've had this discussion many times over. Please see for example the
thread I referenced.

The conclusion has been that our defaults should really allow people to
take backups of their systems, and they currently don't.

Making things run faster is tuning, and people should expect to do that if
they need things to run faster. But being able to make a backup is pretty
fundamental.


Arguing how that isn't a problem looks at least as time consuming as
> fixing the problem.


Please do submit a patch for it. I don't know exactly what's involved in
that part, I just know that people have been complaining about this at
least since 9.0 was released so our track record of actually fixing it
isn't very good.

I'm not arguing that it's not a problem, btw. I'm arguing that until we can
solve the problem, we're much better off letting people do backups and set
up things like replication than optimizing for a usecase that many never
hit.


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

Re: [HACKERS] Replication/backup defaults

[Simon Riggs]

On 2 January 2017 at 09:21, Magnus Hagander  wrote:
>
>
> On Mon, Jan 2, 2017 at 10:17 AM, Simon Riggs  wrote:
>>
>> On 31 December 2016 at 15:00, Magnus Hagander  wrote:
>> > Cycling back to this topic again, but this time at the beginning of a
>> > CF.
>> >
>> > Here's an actual patch to change:
>> >
>> >
>> > max_wal_senders=10
>> > max_replication_slots=20
>>
>> +1
>>
>> If that doesn't fly, it seems easy enough to introduce a
>> "min_reserve_limit" GUC that defaults to 10 that gives a lower bound
>> on the amount of memory we reserve for many of those shmem allocs;
>> that can be set to 0 for people that want the old behaviour. Then we
>> can allow changes up to the memory limit without a restart.
>>
>> > wal_level=replica
>>
>> This is more problematic because it changes behaviours.
>
>
> You can't actually change the other two without changing wal_level.

You could, but we currently disallow it.

>> A more useful approach would be to bring all the things needed to
>> enable replication into one ALTER SYSTEM command, so people have just
>> one thing they need to execute and it will work out the details and
>> locations for you.
>> That way we can maintain the defaults yet make it easier to enable in
>> a useful way.
>
>
> Sure, that would be great - the core being the ability to change these
> things without a restart. But I would argue for not letting perfection get
> in the way of progress, and do this anyway. I doubt there is any way the
> bigger change is going to get done for 10 at this point, so we should give
> people the ability to do backups off a default installation already.

We could fairly easily change wal_level without restart; its been
discussed for many years.

The problem from my perspective is that you're immediately turning off
the performance benefits for initial bulk loads.

Arguing how that isn't a problem looks at least as time consuming as
fixing the problem.

-- 
Simon Riggshttp://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] Replication/backup defaults

[Magnus Hagander]

On Mon, Jan 2, 2017 at 10:17 AM, Simon Riggs  wrote:

> On 31 December 2016 at 15:00, Magnus Hagander  wrote:
> > Cycling back to this topic again, but this time at the beginning of a CF.
> >
> > Here's an actual patch to change:
> >
> >
> > max_wal_senders=10
> > max_replication_slots=20
>
> +1
>
> If that doesn't fly, it seems easy enough to introduce a
> "min_reserve_limit" GUC that defaults to 10 that gives a lower bound
> on the amount of memory we reserve for many of those shmem allocs;
> that can be set to 0 for people that want the old behaviour. Then we
> can allow changes up to the memory limit without a restart.
>
> > wal_level=replica
>
> This is more problematic because it changes behaviours.
>

You can't actually change the other two without changing wal_level.



> A more useful approach would be to bring all the things needed to
> enable replication into one ALTER SYSTEM command, so people have just
> one thing they need to execute and it will work out the details and
> locations for you.
> That way we can maintain the defaults yet make it easier to enable in
> a useful way.
>

Sure, that would be great - the core being the ability to change these
things without a restart. But I would argue for not letting perfection get
in the way of progress, and do this anyway. I doubt there is any way the
bigger change is going to get done for 10 at this point, so we should give
people the ability to do backups off a default installation already.

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

Re: [HACKERS] Replication/backup defaults

[Simon Riggs]

On 31 December 2016 at 15:00, Magnus Hagander  wrote:
> Cycling back to this topic again, but this time at the beginning of a CF.
>
> Here's an actual patch to change:
>
>
> max_wal_senders=10
> max_replication_slots=20

+1

If that doesn't fly, it seems easy enough to introduce a
"min_reserve_limit" GUC that defaults to 10 that gives a lower bound
on the amount of memory we reserve for many of those shmem allocs;
that can be set to 0 for people that want the old behaviour. Then we
can allow changes up to the memory limit without a restart.

> wal_level=replica

This is more problematic because it changes behaviours.

A more useful approach would be to bring all the things needed to
enable replication into one ALTER SYSTEM command, so people have just
one thing they need to execute and it will work out the details and
locations for you.
That way we can maintain the defaults yet make it easier to enable in
a useful way.

> Based on feedback from last year
> (https://www.postgresql.org/message-id/CABUevEwfV7zDutescm2PHGvsJdYA0RWHFMTRGhwrJPGgSbzZDQ%40mail.gmail.com):
>
>
> There were requests for benchmarks of performance difference. Tomas has
> promised to run a couple of benchmarks on his standard benchmarking setups
> to give numbers on that. Thanks Tomas, please pipe in with your results when
> you have them!
>
>
> Security considerations about pg_hba.conf -- I avoided those by not actually
> changing pg_hba.conf. Since pg_hba.conf can be changed on a reload instead
> of a restart it's a lot easier to deal with. I still think changing it to
> allow "postgres" the same type of connections  as it does for regular users
> would not be a security problem, but again thanks to it only needing a
> reload it's not as big an issue.
>
>
> There was the idea to have multiple sets of defaults to choose from at
> initdb time. I don't see a problem having that, but it's been another year
> and nobody built it. I don't think not having that is an excuse for the
> current defaults. And implementing something like that is in no way hindered
> by
> changing the current defaults.
>
> We were too close to beta1 -- this is why I'm sending it earlier this time
> :) (Even though I intended to do it already back in September, better now
> than even later)
>
> Finally, there's the argument that we're already shaking up a number of
> other things with version 10, so this is a good time to do this one as well.
>
> --
>  Magnus Hagander
>  Me: http://www.hagander.net/
>  Work: http://www.redpill-linpro.com/
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



-- 
Simon Riggshttp://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] safer node casting

[Andres Freund]

Hi,


On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
> There is a common coding pattern that goes like this:
> 
> RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
> Assert(IsA(rinfo, RestrictInfo));


> I propose a macro castNode() that combines the assertion and the cast,
> so this would become
> 
> RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

I'm quite a bit in favor of something like this, having proposed it
before ;)

> +#define castNode(_type_,nodeptr) (AssertMacro(!nodeptr || 
> IsA(nodeptr,_type_)), (_type_ *)(nodeptr))

ISTM that we need to do the core part of this in an inline function, to
avoid multiple evaluation hazards - which seem quite likely to occur
here - it's pretty common to cast the result of a function after all.

Something like

static inline Node*
castNodeImpl(void *c, enum NodeTag t)
{
Assert(c == NULL || IsA(c, t));
return c;
}

#define castNode(_type_, nodeptr) ((_type_ *) castNodeImpl(nodeptr, _type_))

should work without too much trouble afaics?

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