date:20160212

There will be necessary more changes than this. Orafce has some parts based
> on lw locks. I am able to compile it without any issue. But the lock
> mechanism is broken now - so impact on extensions will be higher. Have to
> do some research.
>

if somebody would to use it for testing

https://github.com/orafce/orafce
https://github.com/orafce/orafce/commit/fff56ed7e17ed5d6f8e6b71591ff1a6d6ff12d79

With last commit I am able to compile orafce without warnings, but
installcheck is broken. It can be bug in orafce, but this code worked last
7 years.

Pavel


> Regards
>
> Pavel
>
>
>
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>

Re: [HACKERS] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

2016-02-10 17:21 GMT+01:00 Robert Haas :

> On Wed, Feb 10, 2016 at 11:08 AM, Heikki Linnakangas 
> wrote:
> > On 10/02/16 17:12, Robert Haas wrote:
> >> Code cleanup in the wake of recent LWLock refactoring.
> >>
> >> As of commit c1772ad9225641c921545b35c84ee478c326b95e, there's no
> >> longer any way of requesting additional LWLocks in the main tranche,
> >> so we don't need NumLWLocks() or LWLockAssign() any more.  Also,
> >> some of the allocation counters that we had previously aren't needed
> >> any more either.
> >
> > (Sorry if this was discussed already, I haven't been paying attention)
> >
> > LWLockAssign() is used by extensions. Are we OK with just breaking them,
> > requiring them to change LWLockAssign() with the new mechanism, with
> #ifdefs
> > to support multiple server versions? Seems like it shouldn't be too hard
> to
> > keep LWLockAssign() around for the benefit of extensions, so it seems a
> bit
> > inconsiderate to remove it.
>
> It was discussed on the "Refactoring of LWLock tranches" thread,
> though there wasn't an overwhelming consensus or anything.  I don't
> think the burden on extension authors is very much, because you just
> have to do this:
>
> --- a/contrib/pg_stat_statements/pg_stat_statements.c
> +++ b/contrib/pg_stat_statements/pg_stat_statements.c
> @@ -404,7 +404,7 @@ _PG_init(void)
>  * resources in pgss_shmem_startup().
>  */
> RequestAddinShmemSpace(pgss_memsize());
> -   RequestAddinLWLocks(1);
> +   RequestNamedLWLockTranche("pg_stat_statements", 1);
>
> /*
>  * Install hooks.
> @@ -480,7 +480,7 @@ pgss_shmem_startup(void)
> if (!found)
> {
> /* First time through ... */
> -   pgss->lock = LWLockAssign();
> +   pgss->lock =
> &(GetNamedLWLockTranche("pg_stat_statements"))->lock;
> pgss->cur_median_usage = ASSUMED_MEDIAN_INIT;
> pgss->mean_query_len = ASSUMED_LENGTH_INIT;
> SpinLockInit(>mutex);
>
> We've gone through and done this to all of the EnterpriseDB
> proprietary extensions over the last couple of days.
>
> If there's a strong feeling that we should keep the old APIs around,
> we can do that, but I think that (1) if we don't remove them now, we
> probably never will and (2) they are vile APIs.  Allocating the number
> of add-in lwlocks that are requested or a minimum of 3 is just awful.
> If somebody allocates a different number than they request it
> sometimes works, except when combined with some other extension, when
> it maybe doesn't work.  This way, you ask for an LWLock under a given
> name and then get it under that name, so if an extension does it
> wrong, it is that extension that breaks rather than some other one.  I
> think that's enough benefit to justify requiring a small code change
> on the part of extension authors that use LWLocks, but that's
> obviously biased by my experience maintaining EDB's extensions, and
> other people may well feel differently.  But to me, the update that's
> required here is no worse than what
> 5043193b78919a1bd144563aadc2f7f726549913 required, and I didn't hear
> any complaints about that.  You just go through and do to your code
> what got done to the core contrib modules, and you're done.
>

There will be necessary more changes than this. Orafce has some parts based
on lw locks. I am able to compile it without any issue. But the lock
mechanism is broken now - so impact on extensions will be higher. Have to
do some research.

Regards

Pavel



>
> --
> 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] custom function for converting human readable sizes to bytes

2016-02-12 Thread Dean Rasheed

On 12 February 2016 at 06:25, Pavel Stehule  wrote:
> Thank you for review and other work
>

This seems like a reasonable first patch for me as a committer, so
I'll take it unless anyone else was planning to do so.

Regards,
Dean

-- 
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] GinPageIs* don't actually return a boolean

2016-02-12 Thread Yury Zhuravlev


Yury Zhuravlev wrote:

Robert Haas wrote:

So, is it being pulled in indirectly by some other #include?


I can double-check it tomorrow.



I've found who include stdbool.h and think it is inevitable for MSVC2013 
and MSVC2015.

In port/win32.h we inlcude process.h.
In process.h included corecrt_startup.h.
In corecrt_startup.h included vcruntime_startup.h.
In vcruntime_startup.h we included stdbool.h tadam!


--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] Optimization for updating foreign tables in Postgres FDW

2016-02-12 Thread Etsuro Fujita


Hi Rushabh and Thom,

Thanks for the review!

On 2016/02/10 22:37, Thom Brown wrote:

On 10 February 2016 at 08:00, Rushabh Lathia  wrote:

Fujita-san, I am attaching update version of the patch, which added
the documentation update.


Thanks for updating the patch!


Once we finalize this, I feel good with the patch and think that we
could mark this as ready for committer.



I find this wording a bit confusing:

"If the IsForeignRelUpdatable pointer is set to NULL, foreign tables
are assumed to be insertable, updatable, or deletable either the FDW
provides ExecForeignInsert,ExecForeignUpdate, or ExecForeignDelete
respectively or if the FDW optimizes a foreign table update on a
foreign table using PlanDMLPushdown (PlanDMLPushdown still needs to
provide BeginDMLPushdown, IterateDMLPushdown and EndDMLPushdown to
execute the optimized update.)."

This is a very long sentence, and the word "either" doesn't work here.


Agreed.

As a result of our discussions, we reached a conclusion that the DML 
pushdown APIs should be provided together with existing APIs such as 
ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete, IIUC.  So, 
how about (1) leaving the description for the existing APIs as-is and 
(2) adding a new description for the DML pushdown APIs in parenthesis, 
like this?:


 If the IsForeignRelUpdatable pointer is set to
 NULL, foreign tables are assumed to be insertable, 
updatable,

 or deletable if the FDW provides ExecForeignInsert,
 ExecForeignUpdate, or ExecForeignDelete
 respectively.
 (If the FDW attempts to optimize a foreign table update, it still
 needs to provide PlanDMLPushdown, BeginDMLPushdown,
 IterateDMLPushdown and EndDMLPushdown.)

Actually, if the FDW provides the DML pushdown APIs, (pushdown-able) 
foreign table updates can be done without ExecForeignInsert, 
ExecForeignUpdate or ExecForeignDelete.  So, the above docs are not 
necessarily correct.  But we don't recommend to do that without the 
existing APIs, so I'm not sure it's worth complicating the docs.



Also:

"When the query doesn't has the clause, the FDW must also increment
the row count for the ForeignScanState node in the EXPLAIN ANALYZE
case."

Should read "doesn't have"


Will fix.

Best regards,
Etsuro Fujita




--
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] GinPageIs* don't actually return a boolean

On Thu, Feb 11, 2016 at 9:30 PM, Michael Paquier
 wrote:
> On Fri, Feb 12, 2016 at 3:45 AM, Tom Lane  wrote:
>> Andres Freund  writes:
>>> On 2016-02-11 13:37:17 -0500, Tom Lane wrote:
 Absolutely; they don't work safely for testing bits that aren't in the
 rightmost byte of a flag word, for instance.  I'm on board with making
 these fixes, I'm just unconvinced that stdbool is a good reason for it.
>>
>>> Oh, ok. Interactions with stdbool was what made me looking into this,
>>> that's primarily why I mentioned it.   What's your thinking about
>>> back-patching, independent of that then?
>>
>> Well, Yury was saying upthread that some MSVC versions have a problem
>> with the existing coding, which would be a reason to back-patch ...
>> but I'd like to see a failing buildfarm member first.  Don't particularly
>> want to promise to support compilers not represented in the farm.
>
> Grmbl. Forgot to attach the rebased patch upthread. Here is it now.
>
> As of now the only complain has been related to MS2015 and MS2013. If
> we follow the pattern of cec8394b and [1], support to compile on newer
> versions of MSVC would be master and REL9_5_STABLE, but MS2013 is
> supported down to 9.3. Based on this reason, we would want to
> backpatch down to 9.3 the patch of this thread.

OK, that seems reasonable from here.  What I'm still fuzzy about is
why including stdbool.h causes a failure. Is it because it defines a
type called "bool" that clashes with ours?  That seems like the
obvious explanation, but then how does that not cause the compiler to
just straight-up error out?

-- 
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] Optimization for updating foreign tables in Postgres FDW

On Fri, Feb 12, 2016 at 7:19 AM, Etsuro Fujita
 wrote:
> I think that displaying target lists would be confusing for users.  Here is
> an example:
>
> EXPLAIN (verbose, costs off)
> DELETE FROM rem1; -- can be pushed down
>  QUERY PLAN
> -
>  Delete on public.rem1
>->  Foreign Delete on public.rem1
>  Output: ctid
>  Remote SQL: DELETE FROM public.loc1
> (4 rows)
>
> Should we output the "Output" line?

I see your point, but what if there's a RETURNING clause?

-- 
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] Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby

2016-02-12 Thread Michael Paquier

On Thu, Feb 11, 2016 at 5:46 PM, Michael Paquier
 wrote:
> On Thu, Feb 11, 2016 at 5:20 PM, Andres Freund  wrote:
>> On 2016-02-11 09:25:30 +0530, Amit Kapila wrote:
>>> On Wed, Feb 10, 2016 at 1:42 PM, Michael Paquier 
>>> wrote:
>>> >
>>> > On Wed, Feb 10, 2016 at 5:00 PM, Amit Kapila 
>>> wrote:
>>> > > Okay, but isn't it better that we remove the snapshot taken
>>> > > at checkpoint time in the main branch or till where this code is
>>> > > getting back-patched.   Do you see any need of same after
>>> > > having the logging of snapshot in bgwriter?
>>> >
>>> > But this one is necessary as well to allow hot standby faster to
>>> > initialize, no? Particularly in the case where a bgwriter snapshot
>>> > would have been taken just before the checkpoint, there may be up to
>>> > 15s until the next one.
>>> >
>>>
>>> It could be helpful if checkpoint is done at shorter intervals, otherwise
>>> we anyway log it at 15s interval and if need faster initialisation
>>> of hot-standby, then it is better to reduce the log snapshot interval
>>> in bgwriter.
>>
>> No. By emitting the first snapshot directly after the determination of
>> the redo pointer, we can get into STANDBY_SNAPSHOT_READY more
>> quickly. Especially if some of the snapshots are overflowed. During
>> startup 15s can be a *long* time; but on the other hand there's not much
>> benefit at logging snapshots more frequently when the system is up.
>> I don't think we should tinker with the frequency/logging points, while
>> fixing the issue here.
>
> Agreement from here on this point. Andres, what's still remaining on
> my side is to know how to we detect in XLoginsert() how we update the
> LSN progress. I was willing to add XLogInsertExtended() with a
> complementary int/bool flag and let XLogInsert() alone as you don't
> like much doing this decision making using just the record type as I
> did in one of the patch upthread, however you did not like this idea
> either. What exactly do you have in mind?

OK, here is attached a new version that I hope addresses all the
points raised until now. The following things are changed:
- Extend XLogInsert with a new uint8 argument to have flags. As of now
there is one flag: XLOG_INSERT_NO_PROGRESS that can be set to not
update the progress. By default, the progress LSN is updated.
- Add extra check in bgwriter to not log a snapshot to be sure that no
snapshots are logged when there is no activity since last snapshot
taken, and since last checkpoint.
-- 
Michael
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index c740952..22ebba5 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -634,7 +634,7 @@ brinbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 		XLogRegisterData((char *) , SizeOfBrinCreateIdx);
 		XLogRegisterBuffer(0, meta, REGBUF_WILL_INIT);
 
-		recptr = XLogInsert(RM_BRIN_ID, XLOG_BRIN_CREATE_INDEX);
+		recptr = XLogInsert(RM_BRIN_ID, XLOG_BRIN_CREATE_INDEX, 0);
 
 		page = BufferGetPage(meta);
 		PageSetLSN(page, recptr);
diff --git a/src/backend/access/brin/brin_pageops.c b/src/backend/access/brin/brin_pageops.c
index d0ca485..98fcc2c 100644
--- a/src/backend/access/brin/brin_pageops.c
+++ b/src/backend/access/brin/brin_pageops.c
@@ -199,7 +199,7 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
 			XLogRegisterBuffer(0, oldbuf, REGBUF_STANDARD);
 			XLogRegisterBufData(0, (char *) newtup, newsz);
 
-			recptr = XLogInsert(RM_BRIN_ID, info);
+			recptr = XLogInsert(RM_BRIN_ID, info, 0);
 
 			PageSetLSN(oldpage, recptr);
 		}
@@ -294,7 +294,7 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
 			/* old page */
 			XLogRegisterBuffer(2, oldbuf, REGBUF_STANDARD);
 
-			recptr = XLogInsert(RM_BRIN_ID, info);
+			recptr = XLogInsert(RM_BRIN_ID, info, 0);
 
 			PageSetLSN(oldpage, recptr);
 			PageSetLSN(newpage, recptr);
@@ -444,7 +444,7 @@ brin_doinsert(Relation idxrel, BlockNumber pagesPerRange,
 
 		XLogRegisterBuffer(1, revmapbuf, 0);
 
-		recptr = XLogInsert(RM_BRIN_ID, info);
+		recptr = XLogInsert(RM_BRIN_ID, info, 0);
 
 		PageSetLSN(page, recptr);
 		PageSetLSN(BufferGetPage(revmapbuf), recptr);
diff --git a/src/backend/access/brin/brin_revmap.c b/src/backend/access/brin/brin_revmap.c
index b2c273d..d30345f 100644
--- a/src/backend/access/brin/brin_revmap.c
+++ b/src/backend/access/brin/brin_revmap.c
@@ -487,7 +487,7 @@ revmap_physical_extend(BrinRevmap *revmap)
 
 		XLogRegisterBuffer(1, buf, REGBUF_WILL_INIT);
 
-		recptr = XLogInsert(RM_BRIN_ID, XLOG_BRIN_REVMAP_EXTEND);
+		recptr = XLogInsert(RM_BRIN_ID, XLOG_BRIN_REVMAP_EXTEND, 0);
 		PageSetLSN(metapage, recptr);
 		PageSetLSN(page, recptr);
 	}
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 06ba9cb..db2f959 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -415,7

[HACKERS] Small PATCH: check of 2 Perl modules

2016-02-12 Thread Eugene Kazakov


TAP-tests need two Perl modules: Test::More and IPC::Run.

The patch adds checking of modules in configure.in and configure.


Eugene Kazakov,
Postgres Professional

diff --git a/configure b/configure
index 1903815..da6ed4b 100755
--- a/configure
+++ b/configure
@@ -15554,6 +15554,8 @@ done
   if test -z "$PERL"; then
 as_fn_error $? "Perl not found" "$LINENO" 5
   fi
+  AX_PROG_PERL_MODULES( Test::More, , as_fn_error $? "Need Perl Test::More module " "$LINENO" 5)
+  AX_PROG_PERL_MODULES( IPC::Run, , as_fn_error $? "Need Perl IPC::Run module " "$LINENO" 5)
 fi
 
 # Thread testing
diff --git a/configure.in b/configure.in
index 0bd90d7..17e0caa 100644
--- a/configure.in
+++ b/configure.in
@@ -2055,6 +2055,8 @@ if test "$enable_tap_tests" = yes; then
   if test -z "$PERL"; then
 AC_MSG_ERROR([Perl not found])
   fi
+  AX_PROG_PERL_MODULES( Test::More, , AC_MSG_ERROR( [Need Perl Test::More module] ))
+  AX_PROG_PERL_MODULES( IPC::Run, , AC_MSG_ERROR( [Need Perl IPC::Run module] ))
 fi
 
 # Thread testing

-- 
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] Optimization for updating foreign tables in Postgres FDW

2016-02-12 Thread Etsuro Fujita


Hi Robert,

Thanks for the review!

On 2016/02/11 5:43, Robert Haas wrote:

On Wed, Feb 10, 2016 at 3:00 AM, Rushabh Lathia
 wrote:

Fujita-san, I am attaching update version of the patch, which added
the documentation update.

Once we finalize this, I feel good with the patch and think that we
could mark this as ready for committer.



It would be nice to hear from Tom and/or Stephen whether the changes
that have been made since they last commented on it.  I feel like the
design is reasonably OK, but I don't want to push this through if
they're still not happy with it.  One thing I'm not altogether keen on
is the use of "pushdown" or "dml pushdown" as a term of art; on the
other hand, I'm not sure what other term would be better.


I'm open to that naming.  Proposals are welcome!


Comments on specific points follow.

This seems to need minor rebasing in the wake of the join pushdown patch.


Will do.


+   /* Likewise for ForeignScan that has pushed down INSERT/UPDATE/DELETE */
+   if (IsA(plan, ForeignScan) &&
+   (((ForeignScan *) plan)->operation == CMD_INSERT ||
+((ForeignScan *) plan)->operation == CMD_UPDATE ||
+((ForeignScan *) plan)->operation == CMD_DELETE))
+   return;

I don't really understand why this is a good idea.  Since target lists
are only displayed with EXPLAIN (VERBOSE), I don't really understand
what is to be gained by suppressing them in any case at all, and
therefore I don't understand why it's a good idea to do it here,
either.  If there is a good reason, maybe the comment should explain
what it is.


I think that displaying target lists would be confusing for users.  Here 
is an example:


EXPLAIN (verbose, costs off)
DELETE FROM rem1; -- can be pushed down
 QUERY PLAN
-
 Delete on public.rem1
   ->  Foreign Delete on public.rem1
 Output: ctid
 Remote SQL: DELETE FROM public.loc1
(4 rows)

Should we output the "Output" line?


+   /* Check point 1 */
+   if (operation == CMD_INSERT)
+   return false;
+
+   /* Check point 2 */
+   if (nodeTag(subplan) != T_ForeignScan)
+   return false;
+
+   /* Check point 3 */
+   if (subplan->qual != NIL)
+   return false;
+
+   /* Check point 4 */
+   if (operation == CMD_UPDATE)

These comments are referring to something in the function header
further up, but you could instead just delete the stuff from the
header and mention the actual conditions here.  Also:


Will fix.


- If the first condition is supposed accept only CMD_UPDATE or
CMD_DELETE, say if (operation != CMD_UPDATE || operation !=
CMD_DELETE) rather than doing it this way.  Is-not-insert may in this
context be functionally equivalent to is-update-or-delete, but it's
better to write the comment and the code so that they exactly match
rather than kinda-match.

- For point 2, use IsA(subplan, ForiegnScan).


Will fix.


+   /*
+* ignore subplan if the FDW pushes down the
command to the remote
+* server
+*/

This comment states what the code does, instead of explaining why it
does it.  Please update it so that it explains why it does it.


Will update.


+   List   *fdwPushdowns;   /* per-target-table FDW
pushdown flags */

Isn't a list of booleans an awfully inefficient representation?  How
about a Bitmapset?


OK, will do.


+   /*
+* Prepare remote-parameter expressions for evaluation.  (Note: in
+* practice, we expect that all these expressions will be just
Params, so
+* we could possibly do something more efficient than using the full
+* expression-eval machinery for this.  But probably there
would be little
+* benefit, and it'd require postgres_fdw to know more than is desirable
+* about Param evaluation.)
+*/
+   dpstate->param_exprs = (List *)
+   ExecInitExpr((Expr *) fsplan->fdw_exprs,
+(PlanState *) node);

This is an exact copy of an existing piece of code and its associated
comment.  A good bit of the surrounding code is copied, too.  You need
to refactor to avoid duplication, like by putting some of the code in
a new function that both postgresBeginForeignScan and
postgresBeginForeignModify can call.

execute_dml_stmt() has some of the same disease.


Will do.

Best regards,
Etsuro Fujita




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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

On Thu, Feb 11, 2016 at 11:34 PM, Tom Lane  wrote:
> We're not out of the woods on this :-( ... jaguarundi, which is the first
> of the CLOBBER_CACHE_ALWAYS animals to run these tests, didn't like them
> at all.  I think I fixed the deadlock-soft-2 failure, but its take on
> deadlock-hard is:
>
> *** 17,25 
>   step s6a7: LOCK TABLE a7; 
>   step s7a8: LOCK TABLE a8; 
>   step s8a1: LOCK TABLE a1; 
> - step s8a1: <... completed>
>   step s7a8: <... completed>
> ! error in steps s8a1 s7a8: ERROR:  deadlock detected
>   step s8c: COMMIT;
>   step s7c: COMMIT;
>   step s6a7: <... completed>
> --- 17,25 
>   step s6a7: LOCK TABLE a7; 
>   step s7a8: LOCK TABLE a8; 
>   step s8a1: LOCK TABLE a1; 
>   step s7a8: <... completed>
> ! step s8a1: <... completed>
> ! ERROR:  deadlock detected
>   step s8c: COMMIT;
>   step s7c: COMMIT;
>   step s6a7: <... completed>
>
> The problem here is that when the deadlock detector kills s8's
> transaction, s7a8 is also left free to proceed, so there is a race
> condition as to which query completion will get back to
> isolationtester first.
>
> One grotty way to handle that would be something like
>
> -step "s7a8"{ LOCK TABLE a8; }
> +step "s7a8"{ LOCK TABLE a8; SELECT pg_sleep(5); }
>
> Or we could simplify the locking structure enough so that no other
> transactions are released by the deadlock failure.  I do not know
> exactly what you had in mind to be testing here?

Basically just that the deadlock actually got detected.   That may
sound a bit weak, but considering we had no test for it at all before
this...

-- 
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] [PATCH] Refactoring of LWLock tranches

On Thu, Feb 11, 2016 at 11:03 PM, Amit Kapila  wrote:
> Attached patch changes the name of some of the existing tranches
> as suggested by you upthread.

Committed.  Woohoo!

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

2016-02-12 14:10 GMT+01:00 Robert Haas :

> On Fri, Feb 12, 2016 at 3:33 AM, Pavel Stehule 
> wrote:
> >> There will be necessary more changes than this. Orafce has some parts
> >> based on lw locks. I am able to compile it without any issue. But the
> lock
> >> mechanism is broken now - so impact on extensions will be higher. Have
> to do
> >> some research.
> >
> > if somebody would to use it for testing
> >
> > https://github.com/orafce/orafce
> >
> https://github.com/orafce/orafce/commit/fff56ed7e17ed5d6f8e6b71591ff1a6d6ff12d79
> >
> > With last commit I am able to compile orafce without warnings, but
> > installcheck is broken. It can be bug in orafce, but this code worked
> last 7
> > years.
>
> That's very strange.  It looks to me like you did exactly the right
> thing.  Can you provide any details on how it fails?
>

Looks like some race conditions is there - but I didn't tested it deeper

Pavel


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

Re: [HACKERS] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

2016-02-12 10:37 GMT+01:00 Amit Kapila :

> On Fri, Feb 12, 2016 at 2:03 PM, Pavel Stehule 
> wrote:
>
>>
>>
>> There will be necessary more changes than this. Orafce has some parts
>>> based on lw locks. I am able to compile it without any issue. But the lock
>>> mechanism is broken now - so impact on extensions will be higher. Have to
>>> do some research.
>>>
>>
>> if somebody would to use it for testing
>>
>> https://github.com/orafce/orafce
>>
>> https://github.com/orafce/orafce/commit/fff56ed7e17ed5d6f8e6b71591ff1a6d6ff12d79
>>
>> With last commit I am able to compile orafce without warnings, but
>> installcheck is broken. It can be bug in orafce, but this code worked last
>> 7 years.
>>
>>
> One question regarding your latest commit in orafce:
>
> - RequestAddinShmemSpace(SHMEMMSGSZ);
> +#if PG_VERSION_NUM >= 90600
> +
> + RequestNamedLWLockTranche("orafce", 1);
> +
> +#else
> +
> RequestAddinLWLocks(1);
> +#endif
> +
> + RequestAddinShmemSpace(SHMEMMSGSZ);
> +
>
>

> It seems you have moved request for shared memory
> (RequestAddinShmemSpace()) after requesting locks, any reason
> for same?
>

no, it is only moved after lock request

Pavel


>
> You don't need to change the request for shared memory.
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>

Re: [HACKERS] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

On Fri, Feb 12, 2016 at 3:33 AM, Pavel Stehule  wrote:
>> There will be necessary more changes than this. Orafce has some parts
>> based on lw locks. I am able to compile it without any issue. But the lock
>> mechanism is broken now - so impact on extensions will be higher. Have to do
>> some research.
>
> if somebody would to use it for testing
>
> https://github.com/orafce/orafce
> https://github.com/orafce/orafce/commit/fff56ed7e17ed5d6f8e6b71591ff1a6d6ff12d79
>
> With last commit I am able to compile orafce without warnings, but
> installcheck is broken. It can be bug in orafce, but this code worked last 7
> years.

That's very strange.  It looks to me like you did exactly the right
thing.  Can you provide any details on how it fails?

-- 
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] GinPageIs* don't actually return a boolean

On Fri, Feb 12, 2016 at 9:39 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, Feb 12, 2016 at 8:48 AM, Andres Freund  wrote:
>>> E.g. if you include stdbool.h [ ginStepRight breaks ]
>
>> Ah-ha.  OK, now I get it.  So then I agree we should back-patch this
>> at least as far as 9.3 where MSVC 2013 became a supported platform,
>
> Um, no, that does not follow.  The unanswered question here is why,
> when we *have not* included stdbool.h and *have* typedef'd bool as
> just plain "char", we would get C99 bool behavior.  There is something
> happening there that should not be happening, and I'm not really satisfied
> with the explanation "Microsoft is brain-dead as usual".  I think we
> should dig deeper, because whatever is going on there may have deeper
> effects than we now realize.

http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e...@postgrespro.ru
seems to explain what happens pretty clearly.  We #include something
which #includes something which #includes something which #includes
.  It's not that surprising, is it?  I mean, things with
"std" in the name figure to be commonly-included.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

2016-02-12 15:43 GMT+01:00 Robert Haas :

> On Fri, Feb 12, 2016 at 8:16 AM, Pavel Stehule 
> wrote:
> >> That's very strange.  It looks to me like you did exactly the right
> >> thing.  Can you provide any details on how it fails?
> >
> > Looks like some race conditions is there - but I didn't tested it deeper
>
> Well, OK, so I'm totally willing to work with you to help get this
> straightened out, but I'm not really going to go download orafce and
> debug it for you on company time.  I'm fairly sure that won't win me
> any large awards.
>

I'll do it - just need to finish some other. I hope so this night I'll know
more.

Regards

Pavel

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

Re: [HACKERS] GinPageIs* don't actually return a boolean

2016-02-12 Thread Dmitry Ivanov

> OK, that seems reasonable from here.  What I'm still fuzzy about is
> why including stdbool.h causes a failure. Is it because it defines a
> type called "bool" that clashes with ours?  That seems like the
> obvious explanation, but then how does that not cause the compiler to
> just straight-up error out?

stdbool.h defines the '_Bool' type. The standard says:

C99 and C11 §6.3.1.2/1 “When any scalar value is converted to _Bool, the 
result is 0 if the value compares equal to 0; otherwise, the result is 1.”

It seems that MSVC's bool (_Bool) assignment complies to the standard.

-- 
Dmitry Ivanov
Postgres Professional: http://www.postgrespro.com
Russian Postgres 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] GinPageIs* don't actually return a boolean

On 2016-02-12 07:59:06 -0500, Robert Haas wrote:
> OK, that seems reasonable from here.  What I'm still fuzzy about is
> why including stdbool.h causes a failure. Is it because it defines a
> type called "bool" that clashes with ours?  That seems like the
> obvious explanation, but then how does that not cause the compiler to
> just straight-up error out?

No, that's not the problem. Our own definition is actually
conditional. The problem is that the standard's booleans behave
differently. They only ever contain 0 or 1, even if you assign something
outside of that range - essentially they store !!(right-hand-side).  If
you know compare a boolean with the values returned by one of these
macros you'll get into problems.

E.g. if you include stdbool.h:
Buffer
ginStepRight(Buffer buffer, Relation index, int lockmode)
{
boolisLeaf = GinPageIsLeaf(page);
boolisData = GinPageIsData(page);
...
if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page))
elog(ERROR, "right sibling of GIN page is of different type");

doesn't work correctly because isLeaf/isData contain only 0/1 (due to
the standard's bool behaviour), but the values they're compared to
returns some bit set. Due to integer promotion rules isLeaf is promoted
to an integer and compared... And thus the tests fail.

Andres

-- 
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: fix lock contention for HASHHDR.mutex

On Thu, Feb 11, 2016 at 3:26 PM, Robert Haas  wrote:
> On Thu, Feb 11, 2016 at 9:53 AM, Robert Haas  wrote:
>> The fact that InitLocks() doesn't do this has been discussed before
>> and there's no consensus on changing it.  It is, at any rate, a
>> separate issue.  I'll go through the rest of this patch again now.
>
> I did a little bit of cleanup on this patch and the results are
> attached.  I also did some benchmarking of this version.  I tested
> this on two systems, in each case using five-minute, read-only pgbench
> runs at scale factor 3000, varying the client count and taking the
> median of three runs.  First, I tested it on hydra, a 2-socket,
> 16-processor, 64-thread POWER box.  This is a community resource
> hosted at OSUOSL.  Second, I tested it on cthulhu, an 8-socket,
> 64-processor, 128-thread x86_64 box.  This is an EnterpriseDB
> resource.  Here are the results:
>
> hydra, master vs. patched
> 1 client: 8042.872109 vs. 7839.587491 (-2.5%)
> 64 clients: 213311.852043 vs. 214002.314071 (+0.3%)
> 96 clients: 219551.356907 vs. 221908.397489 (+1.1%)
> 128 clients: 210279.022760 vs. 217974.079171 (+3.7%)
>
> cthulhu, master vs. patched
> 1 client: 3407.705820 vs. 3645.129360 (+7.0%)
> 64 clients: 88667.681890 vs. 82636.914343 (-6.8%)
> 96 clients: 79303.750250 vs. 105442.993869 (+33.0%)
> 128 clients: 74684.510668 vs. 120984.938371 (+62.0%)
>
> Obviously, the high-client count results on cthulhu are stellar.  I'm
> *assuming* that the regressions are just random variation.  I am
> however wondering if it to set the freelist affinity based on
> something other than the hash value, like say the PID, so that the
> same process doesn't keep switching to a different freelist for every
> buffer eviction.  It also strikes me that it's probably quite likely
> that slock_t mutex[NUM_FREELISTS] is a poor way to lay out this data
> in memory.  For example, on a system where slock_t is just one byte,
> most likely all of those mutexes are going to be in the same cache
> line, which means you're going to get a LOT of false sharing.  It
> seems like it would be sensible to define a new struct that contains
> an slock_t, a long, and a HASHELEMENT *, and then make an array of
> those structs.  That wouldn't completely eliminate false sharing, but
> it would reduce it quite a bit.  My guess is that if you did that, you
> could reduce the number of freelists to 8 or less and get pretty much
> the same performance benefit that you're getting right now with 32.
> And that, too, seems likely to be good for single-client performance.

One other thing: I don't see what prevents the nentries count for a
particular freelist from going negative, which would subsequently
cause an Assertion failure.  I wonder if we should restructure the
code so that we keep track of the total number of elements allocated
and the number on each freelist, so that the count of elements =
elements allocation - sum(elements on each freelist).  This would
require a little more code churn but it seems that the result would be
more clearly correct.

-- 
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] GinPageIs* don't actually return a boolean

Robert Haas  writes:
> On Fri, Feb 12, 2016 at 8:48 AM, Andres Freund  wrote:
>> E.g. if you include stdbool.h [ ginStepRight breaks ]

> Ah-ha.  OK, now I get it.  So then I agree we should back-patch this
> at least as far as 9.3 where MSVC 2013 became a supported platform,

Um, no, that does not follow.  The unanswered question here is why,
when we *have not* included stdbool.h and *have* typedef'd bool as
just plain "char", we would get C99 bool behavior.  There is something
happening there that should not be happening, and I'm not really satisfied
with the explanation "Microsoft is brain-dead as usual".  I think we
should dig deeper, because whatever is going on there may have deeper
effects than we now realize.

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: CustomScan in a larger structure (RE: [HACKERS] CustomScan support on readfuncs.c)

On Thu, Feb 11, 2016 at 7:06 PM, Kouhei Kaigai  wrote:
>> -Original Message-
>> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
>> Sent: Thursday, February 11, 2016 1:26 PM
>> To: Kaigai Kouhei(海外 浩平)
>> Cc: Andres Freund; Amit Kapila; pgsql-hackers
>> Subject: Re: CustomScan in a larger structure (RE: [HACKERS] CustomScan 
>> support
>> on readfuncs.c)
>>
>> On Wed, Feb 10, 2016 at 1:25 AM, Kouhei Kaigai  wrote:
>> > It is pretty good!
>> >
>> > The attached patch (primary one) implements the above idea.
>> >
>> > Now ExtensibleNode works as a basis structure of data container,
>> > regardless of CustomScan and ForeignScan.
>> > Also, fdw_private and custom_private are de-defined to Node * type
>> > from List * type. It affected to a few FDW APIs.
>>
>> I extracted the subset of this that just creates the extensible node
>> framework and did some cleanup - the result is attached.  I will
>> commit this if nobody objects.
>>
> I have no objection of course.

Done.

-- 
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: CustomScan in a larger structure (RE: [HACKERS] CustomScan support on readfuncs.c)

Hi,


On 2016-02-10 23:26:20 -0500, Robert Haas wrote:
> I think the part about whacking around the FDW API is a little more
> potentially objectionable to others, so I want to hold off doing that
> unless a few more people chime in with +1.  Perhaps we could start a
> new thread to talk about that specific idea.  This is useful even
> without that, though.

FWIW, I can delete a couple hundred lines of code from citusdb thanks to
this...

A quick questions about what you committed:

> @@ -527,10 +532,17 @@ extern PGDLLIMPORT Node *newNodeMacroHolder;
>   */
>  extern char *nodeToString(const void *obj);
>  
> +struct Bitmapset;/* not to include bitmapset.h here */
> +struct StringInfoData;   /* not to include stringinfo.h here */
> +extern void outToken(struct StringInfoData *str, const char *s);
> +extern void outBitmapset(struct StringInfoData *str,
> +  const struct Bitmapset *bms);
> +
>  /*
>   * nodes/{readfuncs.c,read.c}
>   */
>  extern void *stringToNode(char *str);
> +extern struct Bitmapset *readBitmapset(void);

why exactly did you expose read/writeBitmapset(), and nothing else?
Afaics a lot of the other read routines are also pretty necessary from
the outside?

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] GinPageIs* don't actually return a boolean

On Fri, Feb 12, 2016 at 8:48 AM, Andres Freund  wrote:
> On 2016-02-12 07:59:06 -0500, Robert Haas wrote:
>> OK, that seems reasonable from here.  What I'm still fuzzy about is
>> why including stdbool.h causes a failure. Is it because it defines a
>> type called "bool" that clashes with ours?  That seems like the
>> obvious explanation, but then how does that not cause the compiler to
>> just straight-up error out?
>
> No, that's not the problem. Our own definition is actually
> conditional. The problem is that the standard's booleans behave
> differently. They only ever contain 0 or 1, even if you assign something
> outside of that range - essentially they store !!(right-hand-side).  If
> you know compare a boolean with the values returned by one of these
> macros you'll get into problems.
>
> E.g. if you include stdbool.h:
> Buffer
> ginStepRight(Buffer buffer, Relation index, int lockmode)
> {
> boolisLeaf = GinPageIsLeaf(page);
> boolisData = GinPageIsData(page);
> ...
> if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page))
> elog(ERROR, "right sibling of GIN page is of different type");
>
> doesn't work correctly because isLeaf/isData contain only 0/1 (due to
> the standard's bool behaviour), but the values they're compared to
> returns some bit set. Due to integer promotion rules isLeaf is promoted
> to an integer and compared... And thus the tests fail.

Ah-ha.  OK, now I get it.  So then I agree we should back-patch this
at least as far as 9.3 where MSVC 2013 became a supported platform,
per Michael's remarks, and maybe all the way.  Do you want to do that?
 If not, I'll do it.

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


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

Re: [HACKERS] Incorrect formula for SysV IPC parameters

2016-02-12 Thread Fujii Masao

On Fri, Feb 5, 2016 at 2:17 PM, Kyotaro HORIGUCHI
 wrote:
> At Thu, 4 Feb 2016 21:43:04 +0900, Fujii Masao  wrote 
> in 
>> On Wed, Feb 3, 2016 at 12:51 PM, Kyotaro HORIGUCHI
>>  wrote:
>> > Hello, I found that the formulas to calculate SEMMNI and SEMMNS
>> > are incorrect in 9.2 and later.
>> >
>> > http://www.postgresql.org/docs/9.5/static/kernel-resources.html
>> >
>> > But actually the number of semaphores PostgreSQL needs is
>> > calculated as following in 9.4 and later.
> ...
>> > So, the formula for SEMMNI should be
>> >
>> > ceil((max_connections + autovacuum_max_workers + max_worker_processes + 5) 
>> > / 16)
>> >
>> > and SEMMNS should have the same fix.
>> >
>> >
>> > In 9.3 and 9.2, the documentation says the same thing but
> ...
>> > ceil((max_connections + autovacuum_max_workers + 5) / 16)
>> >
>> > In 9.1, NUM_AUXILIARY_PROCS is 3 so the documentations is correct.
>>
>> Good catch!
>
> Thanks.
>
>> ISTM that you also need to change the descriptions about SEMMNI and SEMMNS
>> under the Table 17-1.
>
> Oops! Thank you for pointing it out.
>
> The original description doesn't mention the magic-number ('5' in
> the above formulas, which previously was '4') so I haven't added
> anything about it.
>
> Process of which the number is limited by max_worker_processes is
> called 'background process' (not 'backend worker') in the
> documentation so I used the name to mention it in the additional
> description.
>
> The difference in the body text for 9.2, 9.3 is only a literal
> '4' to '5' in the formula.

Thanks for updating the patches!

They look good to me except that the formulas don't include the number of
background processes requesting shared memory access, i.e.,
GetNumShmemAttachedBgworkers(), in 9.3. Isn't it better to use the following
formula in 9.3?

  ceil((max_connections + autovacuum_max_workers + number of
background proceses + 5) / 16)

Attached patch uses the above formula for 9.3. I'm thinking to push your
patches to 9.2, 9.4, 9.5, master, also push the attached one to 9.3.
Comments?

Regards,

-- 
Fujii Masao


fix_doc_sysvipc_93.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] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

On Fri, Feb 12, 2016 at 8:16 AM, Pavel Stehule  wrote:
>> That's very strange.  It looks to me like you did exactly the right
>> thing.  Can you provide any details on how it fails?
>
> Looks like some race conditions is there - but I didn't tested it deeper

Well, OK, so I'm totally willing to work with you to help get this
straightened out, but I'm not really going to go download orafce and
debug it for you on company time.  I'm fairly sure that won't win me
any large awards.

-- 
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] GinPageIs* don't actually return a boolean

On 2016-02-12 09:39:20 -0500, Tom Lane wrote:
> Robert Haas  writes:
> > On Fri, Feb 12, 2016 at 8:48 AM, Andres Freund  wrote:
> >> E.g. if you include stdbool.h [ ginStepRight breaks ]
> 
> > Ah-ha.  OK, now I get it.  So then I agree we should back-patch this
> > at least as far as 9.3 where MSVC 2013 became a supported platform,
> 
> Um, no, that does not follow.

Well, these headers are generally buggy, so ...

> The unanswered question here is why,
> when we *have not* included stdbool.h and *have* typedef'd bool as
> just plain "char", we would get C99 bool behavior.  There is something
> happening there that should not be happening, and I'm not really satisfied
> with the explanation "Microsoft is brain-dead as usual".  I think we
> should dig deeper, because whatever is going on there may have deeper
> effects than we now realize.

Well,
http://archives.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e20c%40postgrespro.ru
outlines how stdbool.h gets included. That happens fairly at the
begining of c.h.  Later our definitions are guarded by ifdefs:

#ifndef bool
typedef char bool;
#endif

#ifndef true
#define true((bool) 1)
#endif

#ifndef false
#define false   ((bool) 0)
#endif

So we can lament that MS standard libraries include stdbool.h instead of
using _Bool. But I doubt that's going to buy us super much.

Btw, there's a distinct advantage including stdbool: Compilers actually
generate a fair bit better error messages/warnings in some cases. And
the generated code sometimes is a bit better, too.

Andres

-- 
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] GinPageIs* don't actually return a boolean

Robert Haas  writes:
> On Fri, Feb 12, 2016 at 9:39 AM, Tom Lane  wrote:
>> Um, no, that does not follow.  The unanswered question here is why,
>> when we *have not* included stdbool.h and *have* typedef'd bool as
>> just plain "char", we would get C99 bool behavior.

> http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e...@postgrespro.ru
> seems to explain what happens pretty clearly.  We #include something
> which #includes something which #includes something which #includes
> .  It's not that surprising, is it?

Well, the thing that is scaring me here is allowing a platform-specific
definition of "bool" to be adopted.  If, for example, the compiler
writer decided that that should be int width rather than char width,
all hell would break loose.

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] pg_basebackup vs WAL fetching

2016-02-12 Thread Magnus Hagander

Right now, pg_basebackup can only use replication slots with WAL streaming.
It's intended, I believe, to be used when you use pg_basebackup to set up a
new replica, to close the gap before starting a replica (ref
http://www.postgresql.org/message-id/flat/555dd2b2.7020...@gmx.net).

A bit down that thread there is a discussion about pg_basebackup being able
to use this for standalone backups, specifically -X mode, to make sure
things are not rotated away.

AFAICT this was never done, with the thread ending on a subject of "this is
for a different thread". I couldn't find such a different thread.

I think it would still be a very useful feature. In fact, I think it would
be useful enough that it should be enabled by default (but of course
possible to turn off), in the interest of making the default more difficult
to break.

On the backend side, I think that would just mean adding a parameter
to CREATE_REPLICATION_SLOT
to create the slot ephemeral. And register an exit handler that will remove
it when the backup session disconnects as well (ephemeral slots go away on
crash, but AIUI not on regular shutdown).

With this, it should be useful for both -X fetch and -X stream, as long as
pg_basebackup doesn't connect from the master until it has seen all it's
WAL.

Did I miss a thread somewhere saying this is not a good idea?

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

Re: [HACKERS] extend pgbench expressions with functions

On Fri, Feb 12, 2016 at 5:06 AM, Fabien COELHO  wrote:
> I think that this option is too much bother for the small internal purpose
> at hand.

Yeah, I'm really frustrated with this whole thread.  There's been a
huge amount of discussion on this patch, but I don't really feel like
it's converging towards something I could actually commit.

For example, I just realized that this patch allows values to be
either a double or an integer and extends the operators to handle
double values.  But variables can still only be integers.  Please name
any programming language that has such a restriction.  The only ones I
can think of are command shells, and those at least flatten everything
to string rather than integer so that you can store the value without
loss of precision - just with loss of type-safety.  I think designing
this in this way is quite short-sighted.  I don't think variables
should be explicitly typed but they should be able to store a value of
any type that expression evaluation can generate.

Also, as I said back in November, there's really two completely
separate enhancements in here.  One of them is to support a new data
type (doubles) and the other is to support functions.  Those should
really be separate patches.  If neither of you are willing to split
this patch, I'm not willing to commit it.  Going over half of this
patch at a time and getting it committed is going to be a lot of work
for me, but I'm willing to do it.  I'm not willing to go over the
whole thing at once - that's going to take more time than I have, and
produce what will in my opinion be an inferior commit history.  If
somebody else is willing to commit the whole thing as one patch, I'm
not going to object, but I won't do it myself.

I would not worry too much about the list thing at this point.  I'm
sure something simple is fine for that.  I actually think the patch is
now in decent shape as far as code-cleanliness is concerned, but I'm
not sure we've really looked hard enough at the design.  For example,
I find implementing operators as functions in disguise not to be one
of PostgreSQL's most awesome design decisions, and here we are copying
that into pgbench for, well, no benefit that I can see, really.  Maybe
it's a good idea and maybe it's a bad idea, but how do we know?  That
stuff deserves more discussion.  Code cleanup is good, so I do think
it's good that a lot of effort has been put in there, but I don't
think more code cleanup is what's going to get this patch over the
finish line.

-- 
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] extend pgbench expressions with functions

2016-02-12 Thread Fabien COELHO



Hello Michaël,


Using a pointer to the tail of the list would make the code simple,
and save a couple of lines.


I did that, see v27 attached.

Note that it does not save any lines, because the reverse function is 
removed, but then you need another type to keep the head & tail, the link 
type is not enough, and then you have to manage that stuff in the code. 
Whether it is "simpler" is debatable. It probably costs more tests when 
executed.


However, it really saves having to answer the question "why is the list 
reversed?", which is definitely a win from my point of view:-)



Another thing that could be considered is also to move list.c [...]


I think that this option is too much bother for the small internal purpose 
at hand.


--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index ade1b53..eaa0889 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -796,17 +796,21 @@ pgbench  options  dbname
   Sets variable varname to an integer value calculated
   from expression.
   The expression may contain integer constants such as 5432,
-  references to variables :variablename,
+  double constants such as 3.14159,
+  references to integer variables :variablename,
   and expressions composed of unary (-) or binary operators
-  (+, -, *, /, %)
-  with their usual associativity, and parentheses.
+  (+, -, *, /,
+  %) with their usual associativity, function calls and
+  parentheses.
+   shows the available
+  functions.
  
 
  
   Examples:
 
 \set ntellers 10 * :scale
-\set aid (1021 * :aid) % (10 * :scale) + 1
+\set aid (1021 * random(1, 10 * :scale)) % (10 * :scale) + 1
 
 

@@ -826,66 +830,35 @@ pgbench  options  dbname
  
 
  
-  By default, or when uniform is specified, all values in the
-  range are drawn with equal probability.  Specifying gaussian
-  or  exponential options modifies this behavior; each
-  requires a mandatory parameter which determines the precise shape of the
-  distribution.
- 
+  
+   
+
+ \setrandom n 1 10 or \setrandom n 1 10 uniform
+ is equivalent to \set n random(1, 10) and uses a uniform
+ distribution.
+
+   
 
- 
-  For a Gaussian distribution, the interval is mapped onto a standard
-  normal distribution (the classical bell-shaped Gaussian curve) truncated
-  at -parameter on the left and +parameter
-  on the right.
-  Values in the middle of the interval are more likely to be drawn.
-  To be precise, if PHI(x) is the cumulative distribution
-  function of the standard normal distribution, with mean mu
-  defined as (max + min) / 2.0, with
-
- f(x) = PHI(2.0 * parameter * (x - mu) / (max - min + 1)) /
-(2.0 * PHI(parameter) - 1.0)
-
-  then value i between min and
-  max inclusive is drawn with probability:
-  f(i + 0.5) - f(i - 0.5).
-  Intuitively, the larger parameter, the more
-  frequently values close to the middle of the interval are drawn, and the
-  less frequently values close to the min and
-  max bounds. About 67% of values are drawn from the
-  middle 1.0 / parameter, that is a relative
-  0.5 / parameter around the mean, and 95% in the middle
-  2.0 / parameter, that is a relative
-  1.0 / parameter around the mean; for instance, if
-  parameter is 4.0, 67% of values are drawn from the
-  middle quarter (1.0 / 4.0) of the interval (i.e. from
-  3.0 / 8.0 to 5.0 / 8.0) and 95% from
-  the middle half (2.0 / 4.0) of the interval (second and
-  third quartiles). The minimum parameter is 2.0 for
-  performance of the Box-Muller transform.
- 
+  
+   
+\setrandom n 1 10 exponential 3.0 is equivalent to
+\set n random_exponential(1, 10, 3.0) and uses an
+exponential distribution.
+   
+  
 
- 
-  For an exponential distribution, parameter
-  controls the distribution by truncating a quickly-decreasing
-  exponential distribution at parameter, and then
-  projecting onto integers between the bounds.
-  To be precise, with
-
-f(x) = exp(-parameter * (x - min) / (max - min + 1)) / (1.0 - exp(-parameter))
-
-  Then value i between min and
-  max inclusive is drawn with probability:
-  f(x) - f(x + 1).
-  Intuitively, the larger parameter, the more
-  frequently values close to min are accessed, and the
-  less frequently values close to max are accessed.
-  The closer to 0 parameter, the flatter (more uniform)
-  the access distribution.
-  A crude approximation of the distribution is that the most frequent 1%
-  values in the range, close to min, are drawn
-  parameter% of the time.
-  parameter value must be strictly positive.
+  
+   
+\setrandom n 1 10 gaussian 2.0 is equivalent to
+

Re: [HACKERS] GinPageIs* don't actually return a boolean

Andres Freund  writes:
> On February 12, 2016 5:29:44 PM GMT+01:00, Tom Lane  
> wrote:
>> We should standardize on the "((var & FLAG) != 0)"
>> pattern, which works reliably in all cases.

> That's what the second version of my patch, and I presume Michael's updated 
> one as well, does. I think the only open question is how far to backpatch. 
> While annoying work, I think we should go all the way.

I don't object to that, if someone wants to do the work.  A good argument
for it is that we'd otherwise be laying a nasty trap for future
back-patched bug fixes, which might well rely on the cleaned-up behavior.

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] GinPageIs* don't actually return a boolean

2016-02-12 Thread Yury Zhuravlev


Andres Freund wrote:

Did you read what I wrote?

Read.


That's not correct for char booleans, because the can have different bits set.

Current code support this behavior. But to shoot his leg becomes easier.
!= 0 much better of course. 


Thanks.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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 v4] GSSAPI encryption support

2016-02-12 Thread David Steele

Hi Robbie,

On 2/10/16 4:06 PM, Robbie Harwood wrote:
> Hello friends,
> 
> For your consideration, here is a new version of GSSAPI encryption
> support.  For those who prefer, it's also available on my github:
> https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e

It tried out this patch and ran into a few problems:

1) It didn't apply cleanly to HEAD.  It did apply cleanly on a455878
which I figured was recent enough for testing.  I didn't bisect to find
the exact commit that broke it.

2) While I was able to apply the patch and get it compiled it seemed
pretty flaky - I was only able to logon about 1 in 10 times on average.
 Here was my testing methodology:

a) Build Postgres from a455878 (without your patch), install/configure
Kerberos and get everything working.  I was able the set the auth method
to gss in pg_hba.conf and logon successfully every time.

b) On the same system rebuild Postgres from a455878 including your patch
and attempt authentication.

The problems arose after step 2b.  Sometimes I would try to logon twenty
times without success and sometimes it only take five or six attempts.
I was never able to logon successfully twice in a row.

When not successful the client always output this incomplete message
(without  terminating LF):

psql: expected authentication request from server, but received

From the logs I can see the server is reporting EOF from the client,
though the client does not core dump and prints the above message before
exiting.

I have attached files that contain server logs at DEBUG5 and tcpdump
output for both the success and failure cases.

Please let me know if there's any more information you would like me to
provide.

-- 
-David
da...@pgmasters.net
Postgres Log:

DEBUG:  forked new backend, pid=16531 socket=9
DEBUG:  postgres child[16531]: starting with (
DEBUG:  postgres
DEBUG:  )
DEBUG:  InitPostgres
DEBUG:  my backend ID is 2
DEBUG:  StartTransaction
DEBUG:  name: unnamed; blockState:   DEFAULT; state: INPROGR, 
xid/subid/cid: 0/1/0, nestlvl: 1, children:
DEBUG:  Processing received GSS token of length 667
DEBUG:  gss_accept_sec_context major: 0, minor: 0, outlen: 161, outflags: 1b2
DEBUG:  sending GSS response token of length 161
DEBUG:  sending GSS token of length 161
DEBUG:  CommitTransaction
DEBUG:  name: unnamed; blockState:   STARTED; state: INPROGR, 
xid/subid/cid: 0/1/0, nestlvl: 1, children:
DEBUG:  unexpected EOF on client connection
DEBUG:  shmem_exit(0): 1 before_shmem_exit callbacks to make
DEBUG:  shmem_exit(0): 6 on_shmem_exit callbacks to make
DEBUG:  proc_exit(0): 3 callbacks to make
DEBUG:  exit(0)
DEBUG:  shmem_exit(-1): 0 before_shmem_exit callbacks to make
DEBUG:  shmem_exit(-1): 0 on_shmem_exit callbacks to make
DEBUG:  proc_exit(-1): 0 callbacks to make
DEBUG:  reaping dead processes
DEBUG:  server process (PID 16531) exited with exit code 0

TCP Dump:

16:48:15.541558 IP6 (hlim 64, next-header TCP (6) payload length: 40) ::1.51100 
> ::1.5432: Flags [S], cksum 0x0030 (incorrect -> 0xdcd6), seq 1835476714, win 
43690, options [mss 65476,sackOK,TS val 231098240 ecr 0,nop,wscale 7], length 0
0x:  6000  0028 0640      `(.@
0x0010:     0001      
0x0020:     0001 c79c 1538 6d67 26ea  ...8mg&.
0x0030:    a002  0030  0204 ffc4  .0..
0x0040:  0402 080a 0dc6 4780   0103 0307  ..G.
16:48:15.541566 IP6 (hlim 64, next-header TCP (6) payload length: 40) ::1.5432 
> ::1.51100: Flags [S.], cksum 0x0030 (incorrect -> 0x5a2a), seq 3525598000, 
ack 1835476715, win 43690, options [mss 65476,sackOK,TS val 231098240 ecr 
231098240,nop,wscale 7], length 0
0x:  6000  0028 0640      `(.@
0x0010:     0001      
0x0020:     0001 1538 c79c d224 5b30  .8...$[0
0x0030:  6d67 26eb a012  0030  0204 ffc4  mg&..0..
0x0040:  0402 080a 0dc6 4780 0dc6 4780 0103 0307  ..G...G.
16:48:15.541573 IP6 (hlim 64, next-header TCP (6) payload length: 32) ::1.51100 
> ::1.5432: Flags [.], cksum 0x0028 (incorrect -> 0x2c5c), seq 1835476715, ack 
3525598001, win 342, options [nop,nop,TS val 231098240 ecr 231098240], length 0
0x:  6000  0020 0640      `..@
0x0010:     0001      
0x0020:     0001 c79c 1538 6d67 26eb  ...8mg&.
0x0030:  d224 5b31 8010 0156 0028  0101 080a  .$[1...V.(..
0x0040:  0dc6 4780 0dc6 4780  ..G...G.
16:48:15.541806 IP6 (hlim 64, next-header TCP (6) payload length: 129) 
::1.51100 > ::1.5432: Flags [P.], cksum 0x0089 (incorrect -> 0x553d), seq 
1835476715:1835476812, ack 3525598001, win 342, options

Re: [HACKERS] Code of Conduct plan

Josh Berkus  wrote:
> 1. The Core Team will appoint an exploration committee which will look
> at various proposals (including the one drafted on pgsql-general) for
> CoCs and discuss them.

To follow up on this ...

The Core Team are pleased to announce that Stacey Haysler has accepted
our invitation to chair the exploratory committee on a Postgres Code of
Conduct.  Stacey is very well qualified to do this, since she is a well
known member of the Postgres community and has had an extended career in
human resources, including creation and implementation of
anti-discrimination and anti-harassment policies.

Stacey will be reaching out to potential committee members over the next
few days or weeks.  Once the committee is assembled, they will devise
some way (possibly a new mailing list, though I don't want to pre-judge
it) for the wider community to have input into the discussions.
In the meantime, we ask that people continue to refrain from flooding
pgsql-general or other existing PG lists with CoC-related threads.
There will be a time and a place for those discussions, but not yet.

If you have interest or concerns about this process, you can contact
Stacey at shaysler...@gmail.com or the Core Team at
pgsql-c...@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] GinPageIs* don't actually return a boolean

2016-02-12 Thread Yury Zhuravlev


Andres Freund wrote:
Unless I am missing something major, that doesn't seem to 
achieve all that much. A cast to a char based bool wouldn't 
normalize this to 0 or 1. So you're still not guaranteed to be 
able to do somebool == anotherbool when either are set based on 
such a macro.




In C99 cast to bool return 0 or 1 only. In older compilers nothing changes 
(Now the code is designed to "char == char").
I think this is a good option. But of course to write bool and use char 
strange.


--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] GinPageIs* don't actually return a boolean

On February 12, 2016 5:40:29 PM GMT+01:00, Yury Zhuravlev 
 wrote:
>Andres Freund wrote:
>> Unless I am missing something major, that doesn't seem to 
>> achieve all that much. A cast to a char based bool wouldn't 
>> normalize this to 0 or 1. So you're still not guaranteed to be 
>> able to do somebool == anotherbool when either are set based on 
>> such a macro.
>>
>
>In C99 cast to bool return 0 or 1 only.

Don't you say. That's why I brought all this up.


> In older compilers nothing
>changes 
>(Now the code is designed to "char == char").
>I think this is a good option. But of course to write bool and use char
>
>strange.

Did you read what I wrote? That's not correct for char booleans, because the 
can have different bits set.

Andres

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


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

Re: [HACKERS] Seg fault in pgbench

2016-02-12 Thread Alvaro Herrera

Jeff Janes wrote:
> If I give pgbench an empty file, I get a segfault.
> 
> $ touch empty.sql
> $ src/bin/pgbench/pgbench -T 60 -f empty.sql
> starting vacuum...end.
> Segmentation fault (core dumped)
> 
> This has been since this commit:
> 
> commit 8bea3d2219844887e170471f223ba100b3c17571
> Author: Alvaro Herrera 
> Date:   Wed Jan 27 02:54:22 2016 +0100
> 
> pgbench: improve multi-script support
> 
> 
> I wasn't able to figure out which email thread corresponds to this commit.

Thanks for the report, will fix.

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


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

[HACKERS] Seg fault in pgbench

2016-02-12 Thread Jeff Janes

If I give pgbench an empty file, I get a segfault.

$ touch empty.sql
$ src/bin/pgbench/pgbench -T 60 -f empty.sql
starting vacuum...end.
Segmentation fault (core dumped)

This has been since this commit:

commit 8bea3d2219844887e170471f223ba100b3c17571
Author: Alvaro Herrera 
Date:   Wed Jan 27 02:54:22 2016 +0100

pgbench: improve multi-script support


I wasn't able to figure out which email thread corresponds to this commit.

Thanks,

Jeff


-- 
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] Clock with Adaptive Replacement

2016-02-12 Thread Konstantin Knizhnik

Thank you very much for response.

I am not sure that CART can significantly improve PostgreSQL
performance - I just want to know opinion of community about

CAR/CART and other possible alternative to GCLOCK algorithm.

Looks like it CAR really provides better cache hit ratio and so at some
workloads should increase Postgres performance.
But now amount of memory at servers is large enough to completely keep
most of typical databases in cache.
So time of locating buffer in cache is more critical then time of buffer
eviction.

And here CART doesn't provide any benefits comparing with GCLOCK algorithm.

One of the problems with GCLOCK algorithm from my point of view is that
for large caches, containing larger number of pages locating victim page
can take substantial amount of time, because we have to perform several
turnovers before some count becomes zero. In theory CART can address
this problem because there are not counters - justs single bit per page.

On 12.02.2016 18:55, Robert Haas wrote:

On Thu, Feb 11, 2016 at 4:02 PM, Konstantin Knizhnik
wrote:

What do you think about improving cache replacement clock-sweep algorithm in
PostgreSQL with adaptive version proposed in this article:

http://www-cs.stanford.edu/~sbansal/pubs/fast04.pdf

Are there some well known drawbacks of this approach or it will be
interesting to adopt this algorithm to PostgreSQL and measure it impact om
performance under different workloads?
I find this ten years old thread:

http://www.postgresql.org/message-id/flat/d2jkde$6bg$1...@sea.gmane.org#d2jkde$6bg$1...@sea.gmane.org

but it mostly discus possible patent issues with another algorithm ARC (CAR
is inspired by ARC, but it is different algorithm).
As far as I know there are several problems with current clock-sweep
algorithm in PostgreSQL, especially for very large caches.
May be CAR can address some of them?

Maybe, but the proof of the pudding is in the eating. Just because an
algorithm is smarter, newer, and better in general than our current
algorithm - and really, it wouldn't be hard - doesn't mean that it
will actually solve the problems we care about. A few of my
EnterpriseDB colleagues spent a lot of time benchmarking various
tweaks to our current algorithm last year and were unable to construct
a test case where it sped anything up. If they tried the same tweaks
against the 9.4 source base, they could get a speedup. But 9.5 had
locking improvements around buffer eviction, and with those
improvements committed there was no longer any measurable benefit to
improving the quality of buffer eviction decisions. That's a
surprising result, to me anyway, and somebody else might well find a
test case where a benefit can be shown - but our research was not
successful.

I think it's important to spend time and energy figuring out exactly
what the problems with our current algorithm are. We know in general
terms that usage counts tend to converge to either 5 or 0 and
therefore sometimes evict buffers both at great cost and almost
randomly. But what's a lot less clear is how much that actually hurts
us given that we are relying on the OS cache anyway. It may be that
we need to fix some other things before or after improving the buffer
eviction algorithm before we actually get a performance benefit. I
suspect, for example, that a lot of the problems with large
shared_buffers settings have to do with the bgwriter and checkpointer
behavior rather than with the buffer eviction algorithm; and that
others have to do with cache duplication between PostgreSQL and the
operating system. So, I would suggest (although of course it's up to
you) that you might want to focus on experiments that will help you
understand where the problems are before you plunge into writing code
to fix them.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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: fix lock contention for HASHHDR.mutex

2016-02-12 Thread Aleksander Alekseev

Hello, Robert

> It also strikes me that it's probably quite likely that slock_t
> mutex[NUM_FREELISTS] is a poor way to lay out this data in memory.
> For example, on a system where slock_t is just one byte, most likely
> all of those mutexes are going to be in the same cache line, which
> means you're going to get a LOT of false sharing.  It seems like it
> would be sensible to define a new struct that contains an slock_t, a
> long, and a HASHELEMENT *, and then make an array of those structs.
> That wouldn't completely eliminate false sharing, but it would reduce
> it quite a bit.  My guess is that if you did that, you could reduce
> the number of freelists to 8 or less and get pretty much the same
> performance benefit that you're getting right now with 32. And that,
> too, seems likely to be good for single-client performance.

I experimented for a while trying to fit every spinlock in a separate
cache line. Indeed we can gain some speedup this way. Here are
benchmark results on 12-core server for NUM_LOCK_PARTITIONS = 32 (in
this case difference is more notable):

| FREELISTS | SIZE =  32 | SIZE =  64 | SIZE = 128 | SIZE = 256 |
|---|||||
| 4 |   +25.4%   |   +28.7%   |   +28.4%   |   +28.3%   |
| 8 |   +28.2%   |   +29.4%   |   +31.7%   |   +31.4%   |
|16 |   +29.9%   |   +32.6%   |   +31.6%   |   +30.8%   |
|32 |   +33.7%   |   +34.2%   |   +33.6%   |   +32.6%   |

Here SIZE is short for FREELIST_BUFFER_SIZE (part of a hack I used to
align FREELIST structure, see attached patch). Cache line on this CPU
is 64 bytes according to /sys/devices/system/cpu/cpu0/cache/index0/*

I would like to remind that without these changes we had the following
picture (please note "NLP = 32" column):

pgbench -f pgbench.sql -T 150 -P 1 -c 40 -j 12

 DMN | NLP = 16 | NLP = 32 | NLP = 64 | NLP = 128
-|--|--|--|--
   8 |  +15.1%  |  +28.2%  |  +34.1%  |  +33.7%  
  16 |  +16.6%  |  +30.9%  |  +37.0%  |  +40.8%  
  32 |  +15.1%  |  +33.9%  |  +39.5%  |  +41.9%  
  64 |  +15.0%  |  +31.9%  |  +40.1%  |  +42.9%  
 128 |   +7.7%  |  +24.7%  |  +29.6%  |  +31.6%  

* NLP = NUM_LOCK_PARTITIONS
* DMN = DEFAULT_MUTEXES_NUM

So its +34.2% vs +33.9% and the number of freeLists remains the same.

> I am however wondering if it to set the freelist affinity based on
> something other than the hash value, like say the PID, so that the
> same process doesn't keep switching to a different freelist for every
> buffer eviction.

Also I tried to use PID to determine freeList number:

```
#include 
#include 

...

#define FREELIST_IDX(hctl, hashcode) \
  (IS_PARTITIONED(hctl) ? ((uint32)getpid()) % NUM_FREELISTS : 0)

...

// now nentries could be negative in this case
// Assert(FREELIST(hctl, freelist_idx).nentries > 0);

```

Unfortunately this approach gives +33.9% TPS in best case. Also there
is a possibility that nentries will overflow. So far I don't have a
clear idea how to solve this issue effectively.

I'm not sure if further improvements worth an effort.diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 24a53da..4d9ed3e 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -15,7 +15,7 @@
  * to hash_create.  This prevents any attempt to split buckets on-the-fly.
  * Therefore, each hash bucket chain operates independently, and no fields
  * of the hash header change after init except nentries and freeList.
- * A partitioned table uses a spinlock to guard changes of those two fields.
+ * A partitioned table uses spinlocks to guard changes of those fields.
  * This lets any subset of the hash buckets be treated as a separately
  * lockable partition.  We expect callers to use the low-order bits of a
  * lookup key's hash value as a partition number --- this will work because
@@ -111,6 +111,12 @@
 #define DEF_DIRSIZE			   256
 #define DEF_FFACTOR			   1	/* default fill factor */
 
+/* Number of freelists to be used for a partitioned hash table. */
+#define NUM_FREELISTS			4
+
+// TODO: comment. Should be power of 2.
+// #define FREELIST_IDX_STEP 8
+#define FREELIST_BUFFER_SIZE 32
 
 /* A hash bucket is a linked list of HASHELEMENTs */
 typedef HASHELEMENT *HASHBUCKET;
@@ -118,6 +124,20 @@ typedef HASHELEMENT *HASHBUCKET;
 /* A hash segment is an array of bucket headers */
 typedef HASHBUCKET *HASHSEGMENT;
 
+// TODO: re-check comments
+// TODO: pgindent
+
+// TODO: comment!
+typedef struct
+{
+	slock_t mutex;	/* a spinlock */
+	long nentries;	/* number of entries */
+	HASHELEMENT* freeList; /* lists of free elements */
+} FREELIST;
+
+// TODO: comment
+typedef struct { uint8 x[FREELIST_BUFFER_SIZE]; } FREELISTBUFF;
+
 /*
  * Header structure for a hash table --- contains all changeable info
  *
@@ -128,12 +148,16 @@ typedef HASHBUCKET *HASHSEGMENT;
  */
 struct HASHHDR
 {
-	/* In a partitioned table, take this lock to touch nentries or

Re: [HACKERS] GinPageIs* don't actually return a boolean

On Fri, Feb 12, 2016 at 9:47 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, Feb 12, 2016 at 9:39 AM, Tom Lane  wrote:
>>> Um, no, that does not follow.  The unanswered question here is why,
>>> when we *have not* included stdbool.h and *have* typedef'd bool as
>>> just plain "char", we would get C99 bool behavior.
>
>> http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e...@postgrespro.ru
>> seems to explain what happens pretty clearly.  We #include something
>> which #includes something which #includes something which #includes
>> .  It's not that surprising, is it?
>
> Well, the thing that is scaring me here is allowing a platform-specific
> definition of "bool" to be adopted.  If, for example, the compiler
> writer decided that that should be int width rather than char width,
> all hell would break loose.

That's true, but it doesn't really seem like a reason not to commit
this patch.  I mean, the coding here is (a) dangerous by your own
admission and (b) actually breaks on platforms for which we allege
support.  If we find out that somebody has implemented an int-width
bool we'll have some bigger decisions to make, but I don't see any
particular reason why we've got to make those decisions now.

-- 
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] Clock with Adaptive Replacement

On Thu, Feb 11, 2016 at 4:02 PM, Konstantin Knizhnik
 wrote:
> What do you think about improving cache replacement clock-sweep algorithm in
> PostgreSQL with adaptive version proposed in this article:
>
> http://www-cs.stanford.edu/~sbansal/pubs/fast04.pdf
>
> Are there some well known drawbacks of this approach or it will be
> interesting to adopt this algorithm to PostgreSQL and measure it impact om
> performance under different workloads?
> I find this ten years old thread:
>
> http://www.postgresql.org/message-id/flat/d2jkde$6bg$1...@sea.gmane.org#d2jkde$6bg$1...@sea.gmane.org
>
> but it mostly discus possible patent issues with another algorithm ARC (CAR
> is inspired by ARC,  but it is different algorithm).
> As far as I know there are several problems with current clock-sweep
> algorithm in PostgreSQL, especially for very large caches.
> May be CAR can address some of them?

Maybe, but the proof of the pudding is in the eating.  Just because an
algorithm is smarter, newer, and better in general than our current
algorithm - and really, it wouldn't be hard - doesn't mean that it
will actually solve the problems we care about.  A few of my
EnterpriseDB colleagues spent a lot of time benchmarking various
tweaks to our current algorithm last year and were unable to construct
a test case where it sped anything up.  If they tried the same tweaks
against the 9.4 source base, they could get a speedup.  But 9.5 had
locking improvements around buffer eviction, and with those
improvements committed there was no longer any measurable benefit to
improving the quality of buffer eviction decisions.  That's a
surprising result, to me anyway, and somebody else might well find a
test case where a benefit can be shown - but our research was not
successful.

I think it's important to spend time and energy figuring out exactly
what the problems with our current algorithm are.  We know in general
terms that usage counts tend to converge to either 5 or 0 and
therefore sometimes evict buffers both at great cost and almost
randomly.  But what's a lot less clear is how much that actually hurts
us given that we are relying on the OS cache anyway.  It may be that
we need to fix some other things before or after improving the buffer
eviction algorithm before we actually get a performance benefit.  I
suspect, for example, that a lot of the problems with large
shared_buffers settings have to do with the bgwriter and checkpointer
behavior rather than with the buffer eviction algorithm; and that
others have to do with cache duplication between PostgreSQL and the
operating system.  So, I would suggest (although of course it's up to
you) that you might want to focus on experiments that will help you
understand where the problems are before you plunge into writing code
to fix them.

-- 
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] GinPageIs* don't actually return a boolean

Teodor Sigaev  writes:
> One more option for patch:
> #define GinPageIsLeaf(page)((bool)(GinPageGetOpaque(page)->flags & 
> GIN_LEAF))

I think that's a seriously bad coding pattern to adopt, because it would
work for some people but not others if the flag bit is to the left of the
rightmost byte.  We should standardize on the "((var & FLAG) != 0)"
pattern, which works reliably in all cases.

The pattern "(!!(var & FLAG))" would work too, but I dislike it because
it is not "say what you mean" but more of a cute coding trick to save a
keystroke or two.  People who aren't longtime C coders would have to stop
and think about what it does.

(I'd expect reasonable compilers to generate pretty much the same code
in any of these cases, so that aspect of it shouldn't be an issue.)

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] GinPageIs* don't actually return a boolean

On February 12, 2016 5:15:59 PM GMT+01:00, Teodor Sigaev  
wrote:
>One more option for patch:
>
>#define GinPageIsLeaf(page)((bool)(GinPageGetOpaque(page)->flags &
>GIN_LEAF))
>
>Seems it will work on any platform with built-in bool. But I don't know
>will it 
>work with 'typedef char bool' if high bit will be set.

Unless I am missing something major, that doesn't seem to achieve all that 
much. A cast to a char based bool wouldn't normalize this to 0 or 1. So you're 
still not guaranteed to be able to do somebool == anotherbool when either are 
set based on such a macro.

Andres

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

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

Re: [HACKERS] GinPageIs* don't actually return a boolean

2016-02-12 Thread Teodor Sigaev


One more option for patch:

#define GinPageIsLeaf(page)((bool)(GinPageGetOpaque(page)->flags & 
GIN_LEAF))

Seems it will work on any platform with built-in bool. But I don't know will it 
work with 'typedef char bool' if high bit will be set.



That's true, but it doesn't really seem like a reason not to commit
this patch.  I mean, the coding here is (a) dangerous by your own
admission and (b) actually breaks on platforms for which we allege
support.  If we find out that somebody has implemented an int-width
bool we'll have some bigger decisions to make, but I don't see any
particular reason why we've got to make those decisions now.


+1

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
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] GinPageIs* don't actually return a boolean

On 2016-02-12 09:47:47 -0500, Tom Lane wrote:
> Robert Haas  writes:
> > On Fri, Feb 12, 2016 at 9:39 AM, Tom Lane  wrote:
> >> Um, no, that does not follow.  The unanswered question here is why,
> >> when we *have not* included stdbool.h and *have* typedef'd bool as
> >> just plain "char", we would get C99 bool behavior.
> 
> > http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e...@postgrespro.ru
> > seems to explain what happens pretty clearly.  We #include something
> > which #includes something which #includes something which #includes
> > .  It's not that surprising, is it?
> 
> Well, the thing that is scaring me here is allowing a platform-specific
> definition of "bool" to be adopted.  If, for example, the compiler
> writer decided that that should be int width rather than char width,
> all hell would break loose.

Well, for some reason c.h has been written to allow for that for a long
time. I think it's fairly unlikely that somebody writes a _Bool
implementation where sizeof(_Bool) is bigger than sizeof(char). Although
that'd be, by my reading of the standard. permissible. It just says
6.2.5-2: An object declared as type _Bool is large enough to store the
 values 0 and 1.
6.7.2.1: While the number of bits in a _Bool object is at least
 CHAR_BIT, the width (number of sign and value bits) of a _Bool
 may be just 1 bit.

afaics that's pretty much all said about the size of _Bool, except some
bitfield special cases.

But we also only support e.g. CHAR_BIT = 8, so I'm not super concerned
about _Bool being defined too wide.

Andres

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

Re: CustomScan in a larger structure (RE: [HACKERS] CustomScan support on readfuncs.c)

2016-02-12 Thread Kohei KaiGai

2016-02-13 0:11 GMT+09:00 Robert Haas :
> On Fri, Feb 12, 2016 at 9:56 AM, Andres Freund  wrote:
>> On 2016-02-10 23:26:20 -0500, Robert Haas wrote:
>>> I think the part about whacking around the FDW API is a little more
>>> potentially objectionable to others, so I want to hold off doing that
>>> unless a few more people chime in with +1.  Perhaps we could start a
>>> new thread to talk about that specific idea.  This is useful even
>>> without that, though.
>>
>> FWIW, I can delete a couple hundred lines of code from citusdb thanks to
>> this...
>>
>> A quick questions about what you committed:
>>
>>> @@ -527,10 +532,17 @@ extern PGDLLIMPORT Node *newNodeMacroHolder;
>>>   */
>>>  extern char *nodeToString(const void *obj);
>>>
>>> +struct Bitmapset;/* not to include bitmapset.h here */
>>> +struct StringInfoData;   /* not to include stringinfo.h here */
>>> +extern void outToken(struct StringInfoData *str, const char *s);
>>> +extern void outBitmapset(struct StringInfoData *str,
>>> +  const struct Bitmapset *bms);
>>> +
>>>  /*
>>>   * nodes/{readfuncs.c,read.c}
>>>   */
>>>  extern void *stringToNode(char *str);
>>> +extern struct Bitmapset *readBitmapset(void);
>>
>> why exactly did you expose read/writeBitmapset(), and nothing else?
>> Afaics a lot of the other read routines are also pretty necessary from
>> the outside?
>
> Because that's what KaiGai submitted and I had no reason to
> second-guess it.  I'm happy to expose other stuff along similar lines
> if somebody writes a patch for it.
>
Although we have various service macro in outfuncs.c and readfuncs.c,
it is not a big burden for extension authors to write equivalent code,
except for string and bitmap. On the other hands, string needs _outToken,
bitmap needs _outBitmap. It is not good idea to suggest extensions to
have copy & paste code with no clear reason.

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

[HACKERS] Crash with old Windows on new CPU

2016-02-12 Thread Christian Ullrich


Hello,

I just found a compatibility issue when I was migrating an elderly VM to 
a new host. The VM is running Windows Server 2008 SP2, and it has the 
EDB build of PostgreSQL 9.4.5 on it. (9.4.6 behaves the same.) It is 
also not dependent on running in a VM; it would fail on the hardware as 
well.


Backends (and possibly other processes) crash at the slightest 
provocation, such as "SELECT * FROM pg_stat_activity;" or VACUUM. The 
log says either "exception 0xC005" (segfault) or "exception 
0xC01D" (illegal instruction).


The interesting reason: The old host had a Core-generation CPU, which 
does not support the AVX2 instruction set. The new one has a 
Haswell-generation one, and this one does. The EDB distribution of 9.4 
was built with the Visual Studio 2013 compiler, whose CRT (aka libc) has 
a bug where it uses AVX2 instructions if the *CPU* supports them, but 
does not care whether the *OS* does, and 2008 doesn't. That support was 
added in SP1 for 7/2008R2.


There is a workaround, see 
. 
It consists of a single function call, required only if the OS does not 
support AVX2.


I just tried it, and it appears to work. If there is any interest in 
fixing this, I'll be happy to prepare a patch. (Where would be the best 
place to put a function call from  that has to be done during 
startup of each server process, on Windows only?)


Otherwise, it may be time to update the manual (15.6 Supported 
Platforms) where it says PostgreSQL "can be expected to work on these 
operating systems: [...] Windows (Win2000 SP4 and later), [...]". 
Perhaps we could add "except Windows before 7 SP1/2008R2 SP1 when 
running in x64 mode on Intel CPUs introduced after May 2013 (Haswell and 
later)"?


--
Christian



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

Re: [HACKERS] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

2016-02-12 15:46 GMT+01:00 Pavel Stehule :

>
>
> 2016-02-12 15:43 GMT+01:00 Robert Haas :
>
>> On Fri, Feb 12, 2016 at 8:16 AM, Pavel Stehule 
>> wrote:
>> >> That's very strange.  It looks to me like you did exactly the right
>> >> thing.  Can you provide any details on how it fails?
>> >
>> > Looks like some race conditions is there - but I didn't tested it deeper
>>
>> Well, OK, so I'm totally willing to work with you to help get this
>> straightened out, but I'm not really going to go download orafce and
>> debug it for you on company time.  I'm fairly sure that won't win me
>> any large awards.
>>
>
> I'll do it - just need to finish some other. I hope so this night I'll
> know more.
>

In _PG_init I am creating new tranche by
RequestNamedLWLockTranche("orafce", 1);

Immediately when I try to use this lock

shmem_lock = sh_mem->shmem_lock = &(GetNamedLWLockTranche("orafce"))->lock;

I got a error

ERROR:  XX000: requested tranche is not registered
LOCATION:  GetNamedLWLockTranche, lwlock.c:602

Because the session initialization doesn't finish, then Orafce doesn't work


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

Re: [HACKERS] GinPageIs* don't actually return a boolean

On February 12, 2016 5:29:44 PM GMT+01:00, Tom Lane  wrote:
>  We should standardize on the "((var & FLAG) != 0)"
>pattern, which works reliably in all cases.

That's what the second version of my patch, and I presume Michael's updated one 
as well, does. I think the only open question is how far to backpatch. While 
annoying work, I think we should go all the way.

Andres

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

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

Re: [HACKERS] Fuzzy substring searching with the pg_trgm extension

2016-02-12 Thread Teodor Sigaev


On Thu, Feb 11, 2016 at 9:56 AM, Teodor Sigaev  wrote:

1 - sml_limit to similarity_limit. sml_threshold is difficult to write I
think,
similarity_limit is more simple.


It seems to me that threshold is right word by meaning. sml_threshold is my
choice.


Why abbreviate it like that?  Nobody's going to know that "sml" stands
for "similarity" without consulting the documentation, and that sucks.


Ok, I don't have an objections. I worked a lot on various similarity 
modules and sml becomes usual for me. That's why I was asking.


--
Teodor Sigaev  E-mail: teo...@sigaev.ru
  WWW: http://www.sigaev.ru/


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

Re: [HACKERS] Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby

On 2016-02-12 12:37:35 -0500, Robert Haas wrote:
> On Mon, Feb 8, 2016 at 4:18 AM, Andres Freund  wrote:
> > I'm not really a fan. I'd rather change existing callers to add a
> > 'flags' bitmask argument. Right now there can't really be XLogInserts()
> > in extension code, so that's pretty ok to change.
> 
> Yeah, but to what benefit?  You're just turning a smaller patch into a
> bigger one and requiring churning a bunch of code that wouldn't
> otherwise need to be touched.  I think Michael has a good point.

It has the advantage of not ending up with an extra interface, that
we're otherwise never going to get rid of? If not now, when would we
remove it? Sure it touches a few more lines, but that's entirely trivial
mechanical changes, so what?

Andres


-- 
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] WIP: SCRAM authentication

2016-02-12 Thread David Steele

Hi Michael and Heikki,

On 11/16/15 8:53 AM, Michael Paquier wrote:
> On Sat, Sep 5, 2015 at 9:31 AM, Bruce Momjian wrote:
>> On Fri, Sep  4, 2015 at 04:51:33PM -0400, Stephen Frost wrote:
 Coming in late, but can you explain how multiple passwords allow for
 easier automated credential rotation?  If you have five applications
 with stored passwords, I imagine you can't change them all at once, so
 with multiples you could change it on one, then go to the others and
 change it there, and finally, remove the old password.  Is that the
 process?  I am not realizing that without multiple plasswords, this is a
 hard problem.
>>> That's exactly the process if multiple passwords can be used.  If
>>> there's only one account and one password supported then you have to
>>> change all the systems all at once and that certainly can be a hard
>>> problem.
>>>
>>> One way to deal with this is to have a bunch of different accounts, but
>>> that's certainly not simple either and can get quite painful.
>> OK, for me, if we can explain the benefit for users, it seems worth
>> doing just to allow that.
> Reviving an old thread for a patch still registered in this commit
> fest to make the arguing move on.

I was wondering if this patch was going to be submitted for the 2016-03 CF?

If so I am interesting in testing/reviewing or doing any other work that
would be helpful.

> Supporting multiple verifiers for a single role has IMO clear advantages:
> - help transition to new protocols and decommission of old protocols
> without the need to create alternative roles in the backend when
> switching from one or the other.

Agreed.

> - move on to a more complex password aging system. The patch currently
> submitted allows only one verifier per type and per role so this is
> not a complete system. Still, the new catalog table pg_auth_verifiers
> could be later easily extended based on other aging properties that we
> consider adapted to reach this goal.

Yes, with some careful design the pg_auth_verifiers table could support
multiple passwords using a single verifier in the future.  I think the
major changes would be to ALTER ROLE WITH PASSWORD so coming up with an
extensible syntax is important.

In addition, I would prefer to maintain the current structure of the
pg_authid table and use the rolpassword and rolvaliduntil columns to
store only the md5 verifier which will also be stored in
pg_auth_verifiers.  This would provide a smoother migration path with
the idea that rolpassword and rolvaliduntil will be removed from
pg_authid in a future version of Postgres.

> There are clear concerns about protocol aging and how to facilitate
> the life of users to switch to a new system. Hence, I think that the
> patch could include the following:
> - A compatibility GUC allowed_password_verifiers defaulting to a list
> of verifier protocols that we think are safe. This would be added in
> the patch adding infrastructure for multiple verifiers, with default
> to md5. In the patch adding SCRAM, the value of this parameter is
> changed to md5,scram. If a user create a password verifier with a
> protocol not listed in this parameter we return to him a WARNING.
> ERROR may be too much but opinions are welcome.

It seems like an error would be better here.

> - A superuser cleanup function for pg_auth_verifiers aimed at being
> used by pg_upgrade to do needed cleanup of this catalog based on the
> previous GUC to remove outdated verifiers. Optionally, we could have
> an argument for a list of protocols to clean up.
> Using both things we could leverage the upgrade experience and
> transition between systems. Say even if at some point we decide to
> decommission SCRAM we could reuse the same infrastructure to move on
> to a new major version.

Yes - and although the eventual migration process may not need to be
worked out it its entirety we should have a very good idea of what it's
going to look like as that will inform some of the decisions that need
to be made now.

Please let me know if there's anything I can do to expedite this patch.

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




signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

2016-02-12 17:35 GMT+01:00 Pavel Stehule :

>
>
> 2016-02-12 15:46 GMT+01:00 Pavel Stehule :
>
>>
>>
>> 2016-02-12 15:43 GMT+01:00 Robert Haas :
>>
>>> On Fri, Feb 12, 2016 at 8:16 AM, Pavel Stehule 
>>> wrote:
>>> >> That's very strange.  It looks to me like you did exactly the right
>>> >> thing.  Can you provide any details on how it fails?
>>> >
>>> > Looks like some race conditions is there - but I didn't tested it
>>> deeper
>>>
>>> Well, OK, so I'm totally willing to work with you to help get this
>>> straightened out, but I'm not really going to go download orafce and
>>> debug it for you on company time.  I'm fairly sure that won't win me
>>> any large awards.
>>>
>>
>> I'll do it - just need to finish some other. I hope so this night I'll
>> know more.
>>
>
> In _PG_init I am creating new tranche by
> RequestNamedLWLockTranche("orafce", 1);
>
> Immediately when I try to use this lock
>
> shmem_lock = sh_mem->shmem_lock = &(GetNamedLWLockTranche("orafce"))->lock;
>
> I got a error
>
> ERROR:  XX000: requested tranche is not registered
> LOCATION:  GetNamedLWLockTranche, lwlock.c:602
>
> Because the session initialization doesn't finish, then Orafce doesn't work
>

I am starting to understand - the new design is more strict. The Orafce is
designed to run without registration shared_preload_libraries (it is
possible, but not necessary). But - RequestNamedLWLockTranche is working
only for this use case. Then GetNamedLWLockTranche fails, and all other are
probably consequences because shared memory isn't well initialized. After
setting shared_preload_libraries all tests are running. But I cannot do it
generally.

What is your recommendation for this case? So I have not to use named locks?

Regards

Pavel


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

Re: [HACKERS] Fuzzy substring searching with the pg_trgm extension

On Thu, Feb 11, 2016 at 9:56 AM, Teodor Sigaev  wrote:
>> 1 - sml_limit to similarity_limit. sml_threshold is difficult to write I
>> think,
>> similarity_limit is more simple.
>
> It seems to me that threshold is right word by meaning. sml_threshold is my
> choice.

Why abbreviate it like that?  Nobody's going to know that "sml" stands
for "similarity" without consulting the documentation, and that sucks.

-- 
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] ALTER ROLE SET/RESET for multiple options

2016-02-12 Thread Payal Singh

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

When running gmake installcheck for regression tests, 2 tests are failing:
[vagrant@localhost regress]$ cat 
/home/vagrant/postgresql/src/test/regress/regression.diffs
*** /home/vagrant/postgresql/src/test/regress/expected/int8.out 2016-02-11 
22:41:33.983260509 -0500
--- /home/vagrant/postgresql/src/test/regress/results/int8.out  2016-02-11 
22:51:58.631238323 -0500
***
*** 583,593 
  SELECT  AS to_char_13, to_char(q2, 'L.000')  FROM INT8_TBL;
   to_char_13 |to_char 
  +
! |456.000
! |   4567890123456789.000
! |123.000
! |   4567890123456789.000
! |  -4567890123456789.000
  (5 rows)
  
  SELECT '' AS to_char_14, to_char(q2, 'FM.999') FROM INT8_TBL;
--- 583,593 
  SELECT '' AS to_char_13, to_char(q2, 'L.000')  FROM INT8_TBL;
   to_char_13 |to_char 
  +
! | $  456.000
! | $ 4567890123456789.000
! | $  123.000
! | $ 4567890123456789.000
! | $-4567890123456789.000
  (5 rows)
  
  SELECT '' AS to_char_14, to_char(q2, 'FM.999') FROM INT8_TBL;

==

*** /home/vagrant/postgresql/src/test/regress/expected/numeric.out  
2016-02-11 22:41:33.993260509 -0500
--- /home/vagrant/postgresql/src/test/regress/results/numeric.out   
2016-02-11 22:51:58.865238315 -0500
***
*** 1061,1076 
  SELECT '' AS to_char_16, to_char(val, 'L.099')
FROM num_data;
   to_char_16 |  to_char   
  +
! |   .000
! |   .000
! |  -34338492.21539704700
! |  4.310
! |7799461.4119000
! |  16397.0384910
! |  93901.57763026000
! |  -83028485.000
! |  74881.000
! |  -24926804.04504742000
  (10 rows)
  
  SELECT '' AS to_char_17, to_char(val, 'FM.99')
FROM num_data;
--- 1061,1076 
  SELECT '' AS to_char_16, to_char(val, 'L.099')
FROM num_data;
   to_char_16 |  to_char   
  +
! | $ .000
! | $ .000
! | $-34338492.21539704700
! | $4.310
! | $  7799461.4119000
! | $16397.0384910
! | $93901.57763026000
! | $-83028485.000
! | $74881.000
! | $-24926804.04504742000
  (10 rows)
  
  SELECT  AS to_char_17, to_char(val, 'FM.99')  
FROM num_data;

==


The feature seems to work as described, but is it necessary to enclose multiple 
GUC settings in a parenthesis? This seems a deviation from the usual syntax of 
altering multiple settings separated with comma. 

Will test out more once I receive a response from the author.  

The new status of this patch is: Waiting on Author

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

Re: [HACKERS] Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby

On Mon, Feb 8, 2016 at 4:18 AM, Andres Freund  wrote:
> I'm not really a fan. I'd rather change existing callers to add a
> 'flags' bitmask argument. Right now there can't really be XLogInserts()
> in extension code, so that's pretty ok to change.

Yeah, but to what benefit?  You're just turning a smaller patch into a
bigger one and requiring churning a bunch of code that wouldn't
otherwise need to be touched.  I think Michael has a good point.

-- 
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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

Robert Haas  writes:
> On Thu, Feb 11, 2016 at 11:34 PM, Tom Lane  wrote:
>> The problem here is that when the deadlock detector kills s8's
>> transaction, s7a8 is also left free to proceed, so there is a race
>> condition as to which query completion will get back to
>> isolationtester first.
>> 
>> One grotty way to handle that would be something like
>> 
>> -step "s7a8"{ LOCK TABLE a8; }
>> +step "s7a8"{ LOCK TABLE a8; SELECT pg_sleep(5); }
>> 
>> Or we could simplify the locking structure enough so that no other
>> transactions are released by the deadlock failure.  I do not know
>> exactly what you had in mind to be testing here?

> Basically just that the deadlock actually got detected.   That may
> sound a bit weak, but considering we had no test for it at all before
> this...

I tried fixing it as shown above, and was dismayed to find out that
it didn't work, ie, there was still a difference between the regular
output and the results with CLOBBER_CACHE_ALWAYS.  In the latter case
the printout makes it appear that s7a8 completed before s8a1, which
is nonsensical.

Investigation showed that there are a couple of reasons.  One,
isolationtester's is-it-waiting query takes an insane amount of
time under CLOBBER_CACHE_ALWAYS --- over half a second on my
reasonably new server.  Probing the state of half a dozen blocked
sessions thus takes a while.  Second, once s8 has been booted out
of its transaction, s7 is no longer "blocked" according to
isolationtester's definition (it's doing the pg_sleep query
instead).  Therefore, when we're rechecking all the other blocked
steps after detecting that s8 has become blocked, two things
happen: enough time elapses for the deadlock detector to fire,
and then when we get around to checking s7, we don't see it as
blocked and therefore wait until it finishes.  So s7a8 is reported
first despite the pg_sleep, and would be no matter large a pg_sleep
delay is used.

We could possibly fix this by using a deadlock timeout even higher than
5 seconds, but that way madness lies.

Instead, what I propose we do about this is to change isolationtester
so that once it's decided that a given step is blocked, it no longer
issues the is-it-waiting query for that step; it just assumes that the
step should be treated as blocked.  So all we need do for "backlogged"
steps is check PQisBusy/PQconsumeInput.  That both greatly reduces the
number of is-it-waiting queries that are needed and avoids any flappy
behavior of the answer.

Comments?

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] Compilation warning on 9.5

2016-02-12 Thread Vicky Vergara

Hello:

I am a pgRouting developer.

In my CmakeLists.txt I use this flags:
set(CMAKE_C_FLAGS   "${CMAKE_C_FLAGS}   -std=gnu99 -fPIC -O2 -g -Wall 
-Wconversion -pedantic -fmax-errors=10  -Wmissing-prototypes -frounding-math")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x -fPIC -O2 -g -Wconversion 
-Wall -pedantic -fmax-errors=10 -Wextra  -frounding-math -Wno-deprecated")


for testing I use travis CI framework and test pgRouting using postgresql 9.1 
to 9.5
I follow the instructions for including:
#include "postgres.h"
#include "executor/spi.h"
#include "funcapi.h"
#include "catalog/pg_type.h"
#if PGSQL_VERSION > 92
#include "access/htup_details.h"
#endif
#include "fmgr.h"


I wonder if -std=gnu99 is the correct standard to include postgres.h etc. in 
9.5 
because that standard (and all the flags I am using to generate pgrouting code 
without warnings)
catches the following catches warnings of type conversions on some postgresql 
included files.
This doesn't happen on postgresql 9.1 to 9.4

for example:

In file included from /usr/include/postgresql/9.5/server/postgres.h:47:0,
 from 
/home/travis/build/pgRouting/pgrouting/src/dijkstra/src/many_to_many_dijkstra.c:31:
/usr/include/postgresql/9.5/server/c.h:298:9: warning: ISO C does not support 
‘__int128’ type [-pedantic]
/usr/include/postgresql/9.5/server/c.h:299:18: warning: ISO C does not support 
‘__int128’ type [-pedantic]




In file included from /usr/include/postgresql/9.5/server/port/atomics.h:119:0,
 from /usr/include/postgresql/9.5/server/storage/lwlock.h:19,
 from /usr/include/postgresql/9.5/server/storage/lock.h:18,
 from /usr/include/postgresql/9.5/server/access/genam.h:20,
 from /usr/include/postgresql/9.5/server/nodes/execnodes.h:17,
 from /usr/include/postgresql/9.5/server/executor/execdesc.h:18,
 from /usr/include/postgresql/9.5/server/utils/portal.h:50,
 from /usr/include/postgresql/9.5/server/executor/spi.h:18,
 from 
/home/travis/build/pgRouting/pgrouting/src/dijkstra/src/many_to_many_dijkstra.c:32:
/usr/include/postgresql/9.5/server/port/atomics/generic.h: In function 
‘pg_atomic_add_fetch_u32_impl’:
/usr/include/postgresql/9.5/server/port/atomics/generic.h:238:2: warning: 
conversion to ‘uint32’ from ‘int32’ may change the sign of the result 
[-Wsign-conversion]
/usr/include/postgresql/9.5/server/port/atomics/generic.h: In function 
‘pg_atomic_sub_fetch_u32_impl’:
/usr/include/postgresql/9.5/server/port/atomics/generic.h:247:2: warning: 
conversion to ‘uint32’ from ‘int32’ may change the sign of the result 
[-Wsign-conversion]
/usr/include/postgresql/9.5/server/port/atomics/generic.h: In function 
‘pg_atomic_add_fetch_u64_impl’:
/usr/include/postgresql/9.5/server/port/atomics/generic.h:372:2: warning: 
conversion to ‘long unsigned int’ from ‘int64’ may change the sign of the 
result [-Wsign-conversion]
/usr/include/postgresql/9.5/server/port/atomics/generic.h: In function 
‘pg_atomic_sub_fetch_u64_impl’:
/usr/include/postgresql/9.5/server/port/atomics/generic.h:381:2: warning: 
conversion to ‘long unsigned int’ from ‘int64’ may change the sign of the 
result [-Wsign-conversion]
of course, I can't go and modify  generic.h, c.h which are included when I 
include postgres.h or spi.h, or any of the files
that I include that are of the postgresql project.

I already posted in this mailing list 
http://www.postgresql.org/message-id/bay177-w104ec0b93c9fc5453b04cd8a...@phx.gbl
But talking with my co-developer Steve Woodbri, he suggested this mailing list.


you can see a full travis test here:
https://travis-ci.org/pgRouting/pgrouting/builds/108791787
(I have my own warnings which I am fixing and are very visible from 9.1 to 9.4)

Note1: when pgRouting gets released, all those flags:
-Wall -Wconversion -pedantic -fmax-errors=10  -Wmissing-prototypes
will be removed, and of course those warnings won't show up.




Thanks
Vicky Vergara

Re: [HACKERS] Compilation warning on 9.5

Vicky Vergara  writes:
> I wonder if -std=gnu99 is the correct standard to include postgres.h etc. in 
> 9.5 
> because that standard (and all the flags I am using to generate pgrouting 
> code without warnings)
> catches the following catches warnings of type conversions on some postgresql 
> included files.

It's not -std=gnu99 that's causing those messages, it's -pedantic and
-Wconversion respectively.

> /usr/include/postgresql/9.5/server/c.h:298:9: warning: ISO C does not support 
> __int128 type [-pedantic]
> /usr/include/postgresql/9.5/server/c.h:299:18: warning: ISO C does not 
> support __int128 type [-pedantic]

We're not going to do anything about this one; certainly we won't stop
using int128 where it's available, and there isn't any other apparent way
to suppress the warning.  I doubt that -pedantic is a useful switch in
practice, and this seems to be a particularly unhelpful bit of pedantry.
Consider dropping that flag.

> /usr/include/postgresql/9.5/server/port/atomics/generic.h: In function 
> pg_atomic_add_fetch_u32_impl:
> /usr/include/postgresql/9.5/server/port/atomics/generic.h:238:2: warning: 
> conversion to uint32 from int32 may change the sign of the result 
> [-Wsign-conversion]

According to the gcc manual, inserting explicit casts would silence these;
is that worth doing?  I doubt anyone cares about making all our code
-Wconversion clean, but maybe making the headers clean would be worth
the trouble.

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] proposal: schema PL session variables

2016-02-12 22:41 GMT+01:00 Jim Nasby :

> On 2/12/16 2:58 PM, Pavel Stehule wrote:
>
>> I think that's probably true, but this also shows why we need to consider
>> different PLs too. As it stands right now, the only way to access a
>> variable outside of plpgsql would be to call a plpgsql function, and
>> currently there's no way to make a plpgsql function private. So for this to
>> work, private variables probably need to be exposed directly through the pl
>> handler.
>>
>
>
> Again, I'm not saying this all has to be implemented up front, but there
> needs to be a plan for how it would work.
>
> I think it would be a good idea to start a wiki page on this topic to
> start collecting stuff together.


I'll do it. Thank you for comments

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
>

Re: [HACKERS] ALTER ROLE SET/RESET for multiple options

On Fri, Feb 12, 2016 at 1:35 PM, Payal Singh  wrote:
> The feature seems to work as described, but is it necessary to enclose 
> multiple GUC settings in a parenthesis? This seems a deviation from the usual 
> syntax of altering multiple settings separated with comma.

Well, note that you can say:

ALTER USER bob SET search_path = a, b, c;

I'm not sure how the parentheses help exactly; it seems like there is
an inherit ambiguity either way.

-- 
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] Seg fault in pgbench

2016-02-12 Thread Fabien COELHO




If I give pgbench an empty file, I get a segfault.

$ touch empty.sql
$ src/bin/pgbench/pgbench -T 60 -f empty.sql
starting vacuum...end.
Segmentation fault (core dumped)


Oops, shame on me:-(

I should have tested this one, especially as I tested it for other 
changes in pgbench...


Thanks for the report, and for Alvaro's fix.

--
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] Way to check whether a particular block is on the shared_buffer?

On Thu, Feb 11, 2016 at 9:05 PM, Kouhei Kaigai  wrote:
> Hmm. In my experience, it is often not a productive discussion whether
> a feature is niche or commodity. So, let me change the viewpoint.
>
> We may utilize OS-level locking mechanism here.
>
> Even though it depends on filesystem implementation under the VFS,
> we may use inode->i_mutex lock that shall be acquired during the buffer
> copy from user to kernel, at least, on a few major filesystems; ext4,
> xfs and btrfs in my research. As well, the modified NVMe SSD driver can
> acquire the inode->i_mutex lock during P2P DMA transfer.
>
> Once we can consider the OS buffer is updated atomically by the lock,
> we don't need to worry about corrupted pages, but still needs to pay
> attention to the scenario when updated buffer page is moved to GPU.
>
> In this case, PD_ALL_VISIBLE may give us a hint. GPU side has no MVCC
> infrastructure, so I intend to move all-visible pages only.
> If someone updates the buffer concurrently, then write out the page
> including invisible tuples, PD_ALL_VISIBLE flag shall be cleared because
> updated tuples should not be visible to the transaction which issued
> P2P DMA.
>
> Once GPU met a page with !PD_ALL_VISIBLE, it can return an error status
> that indicates CPU to retry this page again. In this case, this page is
> likely loaded to the shared buffer already, so retry penalty is not so
> much.
>
> I'll try to investigate the implementation in this way.
> Please correct me, if I misunderstand something (especially, treatment
> of PD_ALL_VISIBLE).

I suppose there's no theoretical reason why the buffer couldn't go
from all-visible to not-all-visible and back to all-visible again all
during the time you are copying it.

Honestly, I think trying to access buffers without going through
shared_buffers is likely to be very hard to make correct and probably
a loser.  Copying the data into shared_buffers and then to the GPU is,
doubtless, at least somewhat slower.  But I kind of doubt that it's
enough slower to make up for all of the problems you're going to have
with the approach you've chosen.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

On Sat, Feb 13, 2016 at 12:20 AM, Pavel Stehule  wrote:
>> Hmm.  So orafce is actually benefiting from the 3-lwlock slop that was
>> built into the old system: if one of those original 3 locks was
>> as-yet-unclaimed, orafce grabs it when it initializes.  The new system
>> hasn't got that slop, and rather deliberately so.  But that means that
>> the trick that worked before no longer works.
>>
>> It looks to me like the easiest thing to do would be to change the
>> definition of sh_memory so that instead of containing an LWLockId, it
>> contains an LWLock and a tranche ID.  Then the first process to do
>> ShmemInitHash() can call LWLockNewTrancheId() and LWLockInitialize().
>> Every process, first or not, registers the tranche.  Then you don't
>> need GetNamedLWLockTranche().  I think the problem right now is that
>> you can get the shared memory but fail to get the LWLock, and then you
>> have problems... if you put the LWLock in sh_memory, though, that
>> can't happen.
>
>
> The Orafce design is based on old style of shared memory usage. It hasn't
> own segment, and then the pointers are compatible between separate
> processes.

I'm not proposing that you switch to DSM.  There's no real benefit to
that here.  I'm just proposing that (at least in 9.5 and up) you put
the actual LWLock in the chunk of shared memory that you allocate,
rather than just an LWLockId/LWLock *.

> Using new style needs significant refactoring - and I would to
> wait to end of support 9.3. Same situation is with LWLocks - I should to
> share locks with Postmaster and doesn't create own tranche.
>
> In previous design I was able to increase number of Postmaster locks, and
> later, I can take one Postmaster lock. Is it possible?

Not unless we change something.

The actual code changes for what I proposed are not all that large.
But it is a certain amount of work and complexity that I understand is
not appealing to you.

We could back out these changes or invent some kind of backward
compatibility layer.  I don't like that very much, but it could be
done.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

2016-02-13 4:52 GMT+01:00 Robert Haas :

> On Fri, Feb 12, 2016 at 1:08 PM, Pavel Stehule 
> wrote:
> >> I got a error
> >>
> >> ERROR:  XX000: requested tranche is not registered
> >> LOCATION:  GetNamedLWLockTranche, lwlock.c:602
> >>
> >> Because the session initialization doesn't finish, then Orafce doesn't
> >> work
> >
> > I am starting to understand - the new design is more strict. The Orafce
> is
> > designed to run without registration shared_preload_libraries (it is
> > possible, but not necessary). But - RequestNamedLWLockTranche is working
> > only for this use case. Then GetNamedLWLockTranche fails, and all other
> are
> > probably consequences because shared memory isn't well initialized. After
> > setting shared_preload_libraries all tests are running. But I cannot do
> it
> > generally.
> >
> > What is your recommendation for this case? So I have not to use named
> locks?
>
> Hmm.  So orafce is actually benefiting from the 3-lwlock slop that was
> built into the old system: if one of those original 3 locks was
> as-yet-unclaimed, orafce grabs it when it initializes.  The new system
> hasn't got that slop, and rather deliberately so.  But that means that
> the trick that worked before no longer works.
>
> It looks to me like the easiest thing to do would be to change the
> definition of sh_memory so that instead of containing an LWLockId, it
> contains an LWLock and a tranche ID.  Then the first process to do
> ShmemInitHash() can call LWLockNewTrancheId() and LWLockInitialize().
> Every process, first or not, registers the tranche.  Then you don't
> need GetNamedLWLockTranche().  I think the problem right now is that
> you can get the shared memory but fail to get the LWLock, and then you
> have problems... if you put the LWLock in sh_memory, though, that
> can't happen.
>

The Orafce design is based on old style of shared memory usage. It hasn't
own segment, and then the pointers are compatible between separate
processes. Using new style needs significant refactoring - and I would to
wait to end of support 9.3. Same situation is with LWLocks - I should to
share locks with Postmaster and doesn't create own tranche.

In previous design I was able to increase number of Postmaster locks, and
later, I can take one Postmaster lock. Is it possible?

Orafce is multi release code, and I would not to do significant refactoring
when I have not all necessary features on all supported releases. I
understand to fact, so Orafce uses obsolete design, but cannot to change in
this moment (and I would not it if it possible).

Regards

Pavel



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

Re: [HACKERS] Support for N synchronous standby servers - take 2

On Thu, Feb 11, 2016 at 5:40 PM, Michael Paquier
 wrote:
> On Fri, Feb 12, 2016 at 2:56 AM, Robert Haas  wrote:
>> On Fri, Feb 5, 2016 at 3:36 AM, Michael Paquier
>>  wrote:
>>> So, here are some thoughts to make that more user-friendly. I think
>>> that the critical issue here is to properly flatten the meta data in
>>> the custom language and represent it properly in a new catalog,
>>> without messing up too much with the existing pg_stat_replication that
>>> people are now used to for 5 releases since 9.0.
>>
>> Putting the metadata in a catalog doesn't seem great because that only
>> can ever work on the master.  Maybe there's no need to configure this
>> on the slaves and therefore it's OK, but I feel nervous about putting
>> cluster configuration in catalogs.  Another reason for that is that if
>> synchronous replication is broken, then you need a way to change the
>> catalog, which involves committing a write transaction; there's a
>> danger that your efforts to do this will be tripped up by the broken
>> synchronous replication configuration.
>
> I was referring to a catalog view that parses the information related
> to groups of s_s_names in a flattened way to show each group sync
> status. Perhaps my words should have been clearer.

Ah.  Well, that's different, then.

-- 
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] Speed up Clog Access by increasing CLOG buffers

On Fri, Feb 12, 2016 at 12:55 AM, Amit Kapila  wrote:
> Very Good Catch.  I think if we want to address this we can detect
> the non-group leader transactions that tries to update the different
> CLOG page (different from group-leader) after acquiring
> CLogControlLock and then mark these transactions such that
> after waking they need to perform CLOG update via normal path.
> Now this can decrease the latency of such transactions, but I

I think you mean "increase".

> think there will be only very few transactions if at-all there which
> can face this condition, because most of the concurrent transactions
> should be on same page, otherwise the idea of multiple-slots we
> have tried upthread would have shown benefits.
> Another idea could be that we update the comments indicating the
> possibility of multiple Clog-page updates in same group on the basis
> that such cases will be less and even if it happens, it won't effect the
> transaction status update.

I think either approach of those approaches could work, as long as the
logic is correct and the comments are clear.  The important thing is
that the code had better do something safe if this situation ever
occurs, and the comments had better be clear that this is a possible
situation so that someone modifying the code in the future doesn't
think it's impossible, rely on it not happening, and consequently
introduce a very-low-probability bug.

-- 
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] Parallel Aggregate

On Sun, Feb 7, 2016 at 8:21 PM, Haribabu Kommi
 wrote:future.
> Here I attached updated patch with the corrections.

So, what about the main patch, for parallel aggregation itself?  I'm
reluctant to spend too much time massaging combine functions if we
don't have the infrastructure to use them.

This patch removes the comment from float8_regr_sxx in pg_proc.h for
no apparent reason.

-- 
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] Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby

2016-02-12 Thread Michael Paquier

On Sat, Feb 13, 2016 at 1:01 PM, Robert Haas  wrote:
> On Fri, Feb 12, 2016 at 12:56 PM, Andres Freund  wrote:
>> On 2016-02-12 12:37:35 -0500, Robert Haas wrote:
>>> On Mon, Feb 8, 2016 at 4:18 AM, Andres Freund  wrote:
>>> > I'm not really a fan. I'd rather change existing callers to add a
>>> > 'flags' bitmask argument. Right now there can't really be XLogInserts()
>>> > in extension code, so that's pretty ok to change.
>>>
>>> Yeah, but to what benefit?  You're just turning a smaller patch into a
>>> bigger one and requiring churning a bunch of code that wouldn't
>>> otherwise need to be touched.  I think Michael has a good point.
>>
>> It has the advantage of not ending up with an extra interface, that
>> we're otherwise never going to get rid of? If not now, when would we
>> remove it? Sure it touches a few more lines, but that's entirely trivial
>> mechanical changes, so what?

Note: the patch has grown from 15kB to 46kB by switching to the
extended interface to the addition of an argument in XLogInsert().

> I don't feel that there's only one right way to do this, but I think
> Michael's position is both reasonable and similar to what we have done
> in previous cases of this sort.

To be honest, my heart still balances for the Extended() interface.
This reduces the risk of conflicts with back-patching with 9.5.
-- 
Michael


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

Re: [HACKERS] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

2016-02-13 6:32 GMT+01:00 Robert Haas :

> On Sat, Feb 13, 2016 at 12:20 AM, Pavel Stehule 
> wrote:
> >> Hmm.  So orafce is actually benefiting from the 3-lwlock slop that was
> >> built into the old system: if one of those original 3 locks was
> >> as-yet-unclaimed, orafce grabs it when it initializes.  The new system
> >> hasn't got that slop, and rather deliberately so.  But that means that
> >> the trick that worked before no longer works.
> >>
> >> It looks to me like the easiest thing to do would be to change the
> >> definition of sh_memory so that instead of containing an LWLockId, it
> >> contains an LWLock and a tranche ID.  Then the first process to do
> >> ShmemInitHash() can call LWLockNewTrancheId() and LWLockInitialize().
> >> Every process, first or not, registers the tranche.  Then you don't
> >> need GetNamedLWLockTranche().  I think the problem right now is that
> >> you can get the shared memory but fail to get the LWLock, and then you
> >> have problems... if you put the LWLock in sh_memory, though, that
> >> can't happen.
> >
> >
> > The Orafce design is based on old style of shared memory usage. It hasn't
> > own segment, and then the pointers are compatible between separate
> > processes.
>
> I'm not proposing that you switch to DSM.  There's no real benefit to
> that here.  I'm just proposing that (at least in 9.5 and up) you put
> the actual LWLock in the chunk of shared memory that you allocate,
> rather than just an LWLockId/LWLock *.
>
> > Using new style needs significant refactoring - and I would to
> > wait to end of support 9.3. Same situation is with LWLocks - I should to
> > share locks with Postmaster and doesn't create own tranche.
> >
> > In previous design I was able to increase number of Postmaster locks, and
> > later, I can take one Postmaster lock. Is it possible?
>
> Not unless we change something.
>
> The actual code changes for what I proposed are not all that large.
> But it is a certain amount of work and complexity that I understand is
> not appealing to you.
>
> We could back out these changes or invent some kind of backward
> compatibility layer.  I don't like that very much, but it could be
> done.
>

The compatibility layer can be great. Or the some simple API that allows to
use lock without hard code refactoring.

Regards

Pavel



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

Re: [HACKERS] Crash with old Windows on new CPU

2016-02-12 Thread Christian Ullrich


* Robert Haas wrote:


On Fri, Feb 12, 2016 at 7:26 PM, Christian Ullrich  wrote:



startup_hacks(), I think. Proposed patch attached.


Thanks for the report and patch.  Regrettably I haven't the Windows
knowledge to have any idea whether it's right or wrong, but hopefully
someone who knows Windows will jump in here.


In commitfest now.

--
Christian




--
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] extend pgbench expressions with functions

2016-02-12 Thread Fabien COELHO



Hello Robert,


For example, I just realized that this patch allows values to be
either a double or an integer and extends the operators to handle
double values.  But variables can still only be integers.


Indeed.

[...] at least flatten everything to string rather than integer so that 
you can store the value without loss of precision - just with loss of 
type-safety.  I think designing this in this way is quite short-sighted.


Note that I'm not responsible for this design, which is preexisting. 
Extending variables to be able to store doubles could also be done in 
another patch.


I don't think variables should be explicitly typed but they should be 
able to store a value of any type that expression evaluation can 
generate.


Doubles are not really needed that much, it is just to provide something 
to random_* functions parameter, otherwise it is useless as far as pgbench 
is really concerned.



Also, as I said back in November, there's really two completely
separate enhancements in here.  One of them is to support a new data
type (doubles) and the other is to support functions.


Yep. The first part is precisely the patch I initially submitted 5 CF ago.

Then I'm asked to put more things in it to show that it can indeed handle 
another type. Then I'm told "you should not have done that". What can I 
say?



Those should really be separate patches.


They could.

[...] I find implementing operators as functions in disguise not to be 
one of PostgreSQL's most awesome design decisions, and here we are 
copying that into pgbench for, well, no benefit that I can see, really.


Well, I did that initially, then I was asked to implements operators as 
functions. It probably saves some lines, so it is not too bad, even if the 
benefit is limited.



Maybe it's a good idea and maybe it's a bad idea, but how do we know?


This is just pgbench, a tool for testing performance by running dummy 
transactions, not a production thing, so I think that it really does not 
matter. There is no user visible changes wrt operators.


[...] If neither of you are willing to split this patch, I'm not willing 
to commit it.


If I'm reading you correctly, you would consider committing it:

 - if the function & double stuff are separated ?

 - for the double part, if variables can be double ?

--
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] Small PATCH: check of 2 Perl modules

On Fri, Feb 12, 2016 at 8:20 AM, Eugene Kazakov
 wrote:
> TAP-tests need two Perl modules: Test::More and IPC::Run.
>
> The patch adds checking of modules in configure.in and configure.

Why would we want that?

-- 
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] proposal: schema PL session variables

2016-02-12 Thread Jim Nasby


On 2/10/16 1:29 PM, Pavel Stehule wrote:

I got off list mail with little bit different syntax proposal

CREATE VARIABLE xxx DEFAULT [ PRIVATE ]

I am thinking so more SQL natural is form:

CREATE [ PRIVATE ] VARIABLE xxx ...

There should not be only variables, there can be tables, views,
functions, ...

The "PRIVATE" in this context means - only accessible from current
schema. The syntax is different, than I propose, but the idea is same.


+1


I'm not saying we have to implement packages the same way oracle
did. Or at all.

My point is that there are MAJOR features that packages offer that
we don't have at all, with or without schemas. One of those features
is the idea of private objects. You CAN NOT do the same thing with
permissions either, because public vs private doesn't care one iota
about what role is executing something. They only care about what's
in the call stack.


I don't understand well, and probably I don't explain my ideas well. But
this exactly what I would to implement. The security based on locality,
not based on roles.


+1 as well


 Another problem I have with this is it completely ignores
 public/private session variables. The current claim is
that's not a
 big deal because you can only access the variables from a
PL, but I
 give it 2 days of this being released before people are
asking for a
 way to access the variables directly from SQL. Now you have a
 problem because if you want private variables (which I think is
 pretty important) you're only choice is to use SECDEF
functions,
 which is awkward at best.


While this patch doesn't need to implement SQL access to variables,
I think the design needs to address it.


SQL access to variables needs a) change in SQL parser (with difficult
discussion about syntax) or b) generic get/set functions. @b can be used
in other PL in first iteration.

I afraid to open pandora box and I would to hold the scope of this patch
too small what is possible - to be possible implement it in one release
cycle.


I agree about implementation. I disagree about design.

Right now it appears zero thought has gone into what SQL level access 
would eventually look like. Which means there's a high risk that 
something gets implemented now that we regret later.


So I think adding something like this needs to at least address *how* 
SQL level access would work, *when* it's eventually implemented.

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


--
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] Seg fault in pgbench

2016-02-12 Thread Jeff Janes

On Fri, Feb 12, 2016 at 12:22 PM, Alvaro Herrera
 wrote:
> Jeff Janes wrote:
>> If I give pgbench an empty file, I get a segfault.
>>
>> $ touch empty.sql
>> $ src/bin/pgbench/pgbench -T 60 -f empty.sql
>> starting vacuum...end.
>> Segmentation fault (core dumped)
>
> I fixed this by checking whether the first command is NULL; originally
> this case was handled by checking whether the command list itself was
> NULL.  It could also be fixed by having process_file() return NULL in
> the case of an command-less file, but it seemed more churn for no actual
> reason, since the case is going to be rejected by exiting the program
> anyway.
>

Looks good.  Thanks.


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

Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)

2016-02-12 Thread Joe Conway

On 02/11/2016 04:59 PM, Michael Paquier wrote:
> On Fri, Feb 12, 2016 at 9:18 AM, Bruce Momjian  wrote:
>> No, that is not an improvement --- see my previous comment:
>>
>>> We could get more sophisticated by checking the catalog version number
>>> where the format was changed, but that doesn't seem worth it, and is
>>> overly complex because we get the catalog version number from
>>> pg_controldata, so you would be adding a dependency in ordering of the
>>> pg_controldata entries.
>>
>> By testing for '906', you prevent users from using pg_upgrade to go from
>> one catalog version of 9.6 to a later one.  Few people may want to do
>> it, but it should work.
> 
> OK, I see now. I did not consider the case where people would like to
> get upgrade from a dev version of 9.6 to the latest 9.6 version, just
> the upgrade from a previous major version <= 9.5. Thanks for reminding
> that pg_upgrade needs to support that.

Pushed with Bruce's original patch included.

Joe

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] proposal: schema PL session variables

2016-02-12 Thread Jim Nasby


On 2/12/16 2:58 PM, Pavel Stehule wrote:


So I think adding something like this needs to at least address
*how* SQL level access would work, *when* it's eventually implemented.


I understand - and I agree.

small note: Private variables should not be executed from any SQL,
because SQL has not directly related schema. It can be executed only
from SQL embedded in some object with attached schema - PL functions,
views, constraints, .. So for this use case, the important information
is info about a container. We have to hold info about related schema in
planner/executor context.


I think that's probably true, but this also shows why we need to 
consider different PLs too. As it stands right now, the only way to 
access a variable outside of plpgsql would be to call a plpgsql 
function, and currently there's no way to make a plpgsql function 
private. So for this to work, private variables probably need to be 
exposed directly through the pl handler.


Again, I'm not saying this all has to be implemented up front, but there 
needs to be a plan for how it would work.


I think it would be a good idea to start a wiki page on this topic to 
start collecting stuff together.

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


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

On Fri, Feb 12, 2016 at 4:59 PM, Tom Lane  wrote:
> I wrote:
>> Instead, what I propose we do about this is to change isolationtester
>> so that once it's decided that a given step is blocked, it no longer
>> issues the is-it-waiting query for that step; it just assumes that the
>> step should be treated as blocked.  So all we need do for "backlogged"
>> steps is check PQisBusy/PQconsumeInput.  That both greatly reduces the
>> number of is-it-waiting queries that are needed and avoids any flappy
>> behavior of the answer.
>
> Hmm, that seemed to work fine here, but the buildfarm is not very happy
> with it, and on reflection I guess it's just moving the indeterminacy
> somewhere else.  If we check for completion of a given step, and don't
> wait till it's either completed or known blocked, then we have a race
> condition that can change the order in which completions are reported.
>
> The fundamental problem I fear is that isolationtester is designed around
> the assumption that only its own actions (query issuances) change the
> state of the database.  Trying to use it to test deadlock detection is
> problematic because deadlock-breaking changes the DB state asynchronously.
>
> I think what we have to do is revert that change and dumb down
> deadlock-hard until it produces stable results even on the CLOBBER
> critters.  One thing that'd help is reducing the number of processes
> involved --- AFAICS, testing an 8-way deadlock is not really any more
> interesting than testing, say, 4-way, and that would halve the amount
> of time isolationtester spends figuring out the state.

Maybe we should introduce a way to declare whether a step is expected
to wait or not.  I thought about doing that, and the only reason I
didn't is because I couldn't figure out a reasonable syntax.  But, in
many respects, that would actually be better than the current system
of having isolationtester try to figure it out itself.

-- 
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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

I wrote:
> Instead, what I propose we do about this is to change isolationtester
> so that once it's decided that a given step is blocked, it no longer
> issues the is-it-waiting query for that step; it just assumes that the
> step should be treated as blocked.  So all we need do for "backlogged"
> steps is check PQisBusy/PQconsumeInput.  That both greatly reduces the
> number of is-it-waiting queries that are needed and avoids any flappy
> behavior of the answer.

Hmm, that seemed to work fine here, but the buildfarm is not very happy
with it, and on reflection I guess it's just moving the indeterminacy
somewhere else.  If we check for completion of a given step, and don't
wait till it's either completed or known blocked, then we have a race
condition that can change the order in which completions are reported.

The fundamental problem I fear is that isolationtester is designed around
the assumption that only its own actions (query issuances) change the
state of the database.  Trying to use it to test deadlock detection is
problematic because deadlock-breaking changes the DB state asynchronously.

I think what we have to do is revert that change and dumb down
deadlock-hard until it produces stable results even on the CLOBBER
critters.  One thing that'd help is reducing the number of processes
involved --- AFAICS, testing an 8-way deadlock is not really any more
interesting than testing, say, 4-way, and that would halve the amount
of time isolationtester spends figuring out the state.

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] Seg fault in pgbench

2016-02-12 Thread Alvaro Herrera

Jeff Janes wrote:
> If I give pgbench an empty file, I get a segfault.
> 
> $ touch empty.sql
> $ src/bin/pgbench/pgbench -T 60 -f empty.sql
> starting vacuum...end.
> Segmentation fault (core dumped)

I fixed this by checking whether the first command is NULL; originally
this case was handled by checking whether the command list itself was
NULL.  It could also be fixed by having process_file() return NULL in
the case of an command-less file, but it seemed more churn for no actual
reason, since the case is going to be rejected by exiting the program
anyway.

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

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

Re: [HACKERS] proposal: schema PL session variables

Hi



>> SQL access to variables needs a) change in SQL parser (with difficult
>> discussion about syntax) or b) generic get/set functions. @b can be used
>> in other PL in first iteration.
>>
>> I afraid to open pandora box and I would to hold the scope of this patch
>> too small what is possible - to be possible implement it in one release
>> cycle.
>>
>
> I agree about implementation. I disagree about design.
>
> Right now it appears zero thought has gone into what SQL level access
> would eventually look like. Which means there's a high risk that something
> gets implemented now that we regret later.
>
> So I think adding something like this needs to at least address *how* SQL
> level access would work, *when* it's eventually implemented.


I understand - and I agree.

small note: Private variables should not be executed from any SQL, because
SQL has not directly related schema. It can be executed only from SQL
embedded in some object with attached schema - PL functions, views,
constraints, .. So for this use case, the important information is info
about a container. We have to hold info about related schema in
planner/executor context.

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
>

[HACKERS] innocuous: pgbench does FD_ISSET on invalid socket

2016-02-12 Thread Alvaro Herrera

I noticed that pgbench calls FD_ISSET on a socket returned by
PQsocket() without first checking that it's not invalid.  I don't think
there's a real problem here because the socket was already checked a few
lines above, but I think applying the same coding pattern to both places
is cleaner.

Any objections to changing it like this?  I'd probably backpatch to 9.5,
but no further (even though this pattern actually appears all the way
back.)

*** a/src/bin/pgbench/pgbench.c
--- b/src/bin/pgbench/pgbench.c
***
*** 3770,3780  threadRun(void *arg)
Command   **commands = 
sql_script[st->use_file].commands;
int prev_ecnt = st->ecnt;
  
!   if (st->con && (FD_ISSET(PQsocket(st->con), _mask)
!   || 
commands[st->state]->type == META_COMMAND))
{
!   if (!doCustom(thread, st, ))
!   remains--;  /* I've aborted */
}
  
if (st->ecnt > prev_ecnt && commands[st->state]->type 
== META_COMMAND)
--- 3770,3790 
Command   **commands = 
sql_script[st->use_file].commands;
int prev_ecnt = st->ecnt;
  
!   if (st->con)
{
!   int sock = PQsocket(st->con);
! 
!   if (sock < 0)
!   {
!   fprintf(stderr, "bad socket: %s\n", 
strerror(errno));
!   goto done;
!   }
!   if (FD_ISSET(sock, _mask) ||
!   commands[st->state]->type == 
META_COMMAND)
!   {
!   if (!doCustom(thread, st, ))
!   remains--;  /* I've aborted 
*/
!   }
}
  
if (st->ecnt > prev_ecnt && commands[st->state]->type 
== META_COMMAND)

-- 
Álvaro Herrerahttp://www.linkedin.com/in/alvherre


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

Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)

2016-02-12 Thread Bruce Momjian

On Thu, Feb 11, 2016 at 07:18:46PM -0500, Bruce Momjian wrote:
> No, that is not an improvement --- see my previous comment:
> 
> > We could get more sophisticated by checking the catalog version number
> > where the format was changed, but that doesn't seem worth it, and is
> > overly complex because we get the catalog version number from
> > pg_controldata, so you would be adding a dependency in ordering of the
> > pg_controldata entries.
> 
> By testing for '906', you prevent users from using pg_upgrade to go from
> one catalog version of 9.6 to a later one.  Few people may want to do
> it, but it should work.

C comment added explaining why we do this.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
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] Crash with old Windows on new CPU

2016-02-12 Thread Christian Ullrich


* Christian Ullrich wrote:


Backends (and possibly other processes) crash at the slightest
provocation, such as "SELECT * FROM pg_stat_activity;" or VACUUM. The
log says either "exception 0xC005" (segfault) or "exception
0xC01D" (illegal instruction).

The interesting reason: The old host had a Core-generation CPU, which
does not support the AVX2 instruction set. The new one has a
Haswell-generation one, and this one does. The EDB distribution of 9.4
was built with the Visual Studio 2013 compiler, whose CRT (aka libc) has
a bug where it uses AVX2 instructions if the *CPU* supports them, but
does not care whether the *OS* does, and 2008 doesn't. That support was
added in SP1 for 7/2008R2.



I just tried it, and it appears to work. If there is any interest in
fixing this, I'll be happy to prepare a patch. (Where would be the best
place to put a function call from  that has to be done during
startup of each server process, on Windows only?)


startup_hacks(), I think. Proposed patch attached.

--
Christian
diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index 19176b1..4be21a0 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -26,6 +26,13 @@
 #include 
 #endif
 
+#ifdef _WIN32
+#if _MSC_VER == 1800
+#include 
+#include 
+#endif
+#endif
+
 #include "bootstrap/bootstrap.h"
 #include "common/username.h"
 #include "postmaster/postmaster.h"
@@ -263,6 +270,22 @@ startup_hacks(const char *progname)
 
/* In case of general protection fault, don't show GUI popup 
box */
SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX);
+
+#ifdef _M_AMD64
+#if _MSC_VER == 1800
+   /*
+* Avoid crashing in certain floating-point operations if
+* we were compiled for x64 with MS Visual Studio 2013 and
+* are running on Windows prior to 7/2008R2 SP1 on an 
+* AVX2-capable CPU.
+*/
+   if (!IsWindows7SP1OrGreater())
+   {
+   _set_FMA3_enable(0);
+   }
+#endif /* _MSC_VER == 1800 */
+#endif /* _M_AMD64 */
+
}
 #endif   /* WIN32 */
 

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

Re: [HACKERS] Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby

On Fri, Feb 12, 2016 at 12:56 PM, Andres Freund  wrote:
> On 2016-02-12 12:37:35 -0500, Robert Haas wrote:
>> On Mon, Feb 8, 2016 at 4:18 AM, Andres Freund  wrote:
>> > I'm not really a fan. I'd rather change existing callers to add a
>> > 'flags' bitmask argument. Right now there can't really be XLogInserts()
>> > in extension code, so that's pretty ok to change.
>>
>> Yeah, but to what benefit?  You're just turning a smaller patch into a
>> bigger one and requiring churning a bunch of code that wouldn't
>> otherwise need to be touched.  I think Michael has a good point.
>
> It has the advantage of not ending up with an extra interface, that
> we're otherwise never going to get rid of? If not now, when would we
> remove it? Sure it touches a few more lines, but that's entirely trivial
> mechanical changes, so what?

I don't think that it's a bad thing to have a simple interface for
simple use cases and a complex one for more complex cases.  I don't
feel any need to convert every use of ReadBuffer() in the source code
to ReadBufferExtended(), for example.  Nor do I feel like we should
have changed every call to LockAcquire() instead of introducing
LockAcquireExtended().  This case seems analogous, and there are a few
advantages.  One is that it avoids creating meaningless conflicts when
back-patching.   Another is that when a function gets an extra
argument, that often turns out to be something that happens more than
once.  It's nice not to keep having to whack around every call in the
tree every time the call signature changes.  Also, finding the small
number of callers that need non-default behavior is simplified,
because you can just grep for the Extended version of the function and
there won't be many hits.

I don't feel that there's only one right way to do this, but I think
Michael's position is both reasonable and similar to what we have done
in previous cases of this sort.

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

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

Re: [HACKERS] [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.