Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP
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
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.
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.
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
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
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?
>> 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
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
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
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
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
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]
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
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?
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
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 ha
Re: [HACKERS] Cluster wide option to control symbol case folding
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
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
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
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
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
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
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
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
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, &minXlogTli, &minXlogSegNo);
+
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
> 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
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
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
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
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
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
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
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 &args->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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
