Re: [HACKERS] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Pavel Stehule]

2016-07-06 5:17 GMT+02:00 Peter Eisentraut :

> On 7/2/16 12:40 AM, Pavel Stehule wrote:
>
>> So ANSI SQL conform implementation of JSON support is still possible in
>> Postgres.
>>
>
> It is possible, but in this case I think it's not worth it.  Our built-in
> stuff is significantly simpler and more powerful and already in widespread
> use.
>

I can't to agree. JSONPath is little bit powerful than our JSON path
expression. JSON_TABLE is pretty good function for extracting values from
JSON (note: I missing XML_TABLE too much).

More - Postgres is not famous due not respecting standard.

Regards

Pavel



>
> It is worth keeping an eye on it and not create gratuitous conflicts.
>
>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] can we optimize STACK_DEPTH_SLOP

[Greg Stark]

On Tue, Jul 5, 2016 at 8:48 PM, Tom Lane  wrote:
> Unfortunately the way I did (1) only works on systems with pmap; I'm not
> sure how to make it more portable.

I did a similar(ish) test which is admittedly not as exhaustive as
using pmap. I instrumented check_stack_depth() itself to keep track of
a high water mark (and based on Robert's thought process) to keep
track of the largest increment over the previous checked stack depth.
This doesn't cover any cases where there's no check_stack_depth() call
in the call stack at all (but then if there's no check_stack_depth
call at all it's hard to see how any setting of STACK_DEPTH_SLOP is
necessarily going to help).

I see similar results to you. The regexp test shows:
LOG:  disconnection: highest stack depth: 392256 largest stack increment: 35584

And the:
STATEMENT:  select infinite_recurse();
LOG:  disconnection: highest stack depth: 2097584 largest stack increment: 1936

There were a couple other tests with similar stack increase increments
to the regular expression test:

STATEMENT:  alter table atacc2 add constraint foo check (test>0) no inherit;
LOG:  disconnection: highest stack depth: 39232 largest stack increment: 34224
STATEMENT:  SELECT chr(0);
LOG:  disconnection: highest stack depth: 44144 largest stack increment: 34512

But aside from those two the next largest increment between two
success check_stack_depth calls was about 12kB:

STATEMENT:  select array_elem_check(121.00);
LOG:  disconnection: highest stack depth: 24256 largest stack increment: 12896

This was all on x86_64 too.

-- 
greg
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index b185c1b..b5d7f80 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3120,6 +3120,24 @@ check_stack_depth(void)
}
 }
 
+#ifdef DEBUG_STACK_DEPTH
+long last_stack_depth = 0;
+long high_stack_depth = 0;
+long high_stack_incr = 0;
+bool stack_depth_on_proc_exit = 0;
+
+/*
+ * on_proc_exit handler to log end of session
+ */
+static void
+log_stack_depth(int code, Datum arg)
+{
+   ereport(LOG,
+   (errmsg("disconnection: highest stack depth: %lu 
largest stack increment: %lu",
+   high_stack_depth, high_stack_incr)));
+}
+#endif
+
 bool
 stack_is_too_deep(void)
 {
@@ -3137,6 +3155,21 @@ stack_is_too_deep(void)
if (stack_depth < 0)
stack_depth = -stack_depth;
 
+#ifdef DEBUG_STACK_DEPTH
+   /* book-keeping for measuring STACK_DEPTH_SLOP */
+   if (stack_depth > high_stack_depth)
+   high_stack_depth = stack_depth;
+   if (stack_depth - last_stack_depth > high_stack_incr)
+   high_stack_incr = stack_depth - last_stack_depth;
+   last_stack_depth = stack_depth;
+
+   if (!stack_depth_on_proc_exit)
+   {
+   stack_depth_on_proc_exit = 1;
+   on_proc_exit(log_stack_depth, 0);
+   }
+#endif
+
/*
 * Trouble?
 *

-- 
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] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Peter Eisentraut]

On 7/2/16 12:40 AM, Pavel Stehule wrote:

So ANSI SQL conform implementation of JSON support is still possible in
Postgres.


It is possible, but in this case I think it's not worth it.  Our 
built-in stuff is significantly simpler and more powerful and already in 
widespread use.


It is worth keeping an eye on it and not create gratuitous conflicts.

--
Peter Eisentraut  http://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] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Peter Eisentraut]

On 7/1/16 10:42 PM, Andreas Karlsson wrote:

On 07/01/2016 07:31 PM, Andres Freund wrote:

Yeah, but since when has the SQL standard adopted any existing
implementation's spelling of a feature? It seems to be politically
impossible.


Especially if nobody really lobbies on part of postgresql.


Has any of the major PostgreSQL companies looked into sending members to
the ISO committee? It would be nice if we could be represented.


It has been considered from time to time, but it would require some 
significant time investment from people whose time is better spent 
elsewhere.


--
Peter Eisentraut  http://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] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Peter Eisentraut]

On 7/4/16 12:44 AM, Thomas Munro wrote:

ISO/IEC PDTR 19075-5 (Row Pattern Recognition) has also reached stage
30.60.  Does anyone know what that one is about?  Maybe something like
MATCH_RECOGNIZE in Oracle?


That's exactly what that is.

--
Peter Eisentraut  http://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] Documentation fixes for pg_visibility

[Michael Paquier]

Robert wrote:
>  I think there should probably be a test for
> all_visible_according_to_vm at the beginning of that so that we don't
> add more visibility map checks for pages where we already know the VM
> bit can't possibly be set.

Yes, that looks like a good idea after more screening of this code.

On Wed, Jul 6, 2016 at 12:21 AM, Robert Haas  wrote:
> So I'm a bit confused about what you are saying here.  If the page is
> marked all-frozen but actually isn't all-frozen, then we should clear
> the all-frozen bit in the VM.  The easiest way to do that is to clear
> both bits in the VM plus the page-level bit, as done here, because we
> don't presently have a way of clearing just one of the visibility map
> bits.

Yes, that's my understanding as well for what is necessary: clear both
bits in the vm as well as the bit on the page itself, and mark it as
dirty. Way to go.

> Now, since the heap_lock_tuple issue requires us to introduce a new
> method to clear all-visible without clearing all-frozen, we could
> possibly use that here too, once we have it.

For 10.0, working on lower-level routine optimizations of this kind
sounds good to me. But I vote against this level of code reordering in
9.6 post-beta2 if that's what you suggest.
--
Michael
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 32b6fdd..98cf9e8 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -1183,6 +1183,23 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
 		}
 
 		/*
+		 * If the page contains non-frozen tuples but the visibility map is
+		 * telling the contrary, inform of the situation and take preventive
+		 * measures on the page and the visibility map. This situation should
+		 * not happen however.
+		 */
+		else if (all_visible_according_to_vm &&
+ VM_ALL_FROZEN(onerel, blkno, ) &&
+ !all_frozen)
+		{
+			elog(WARNING, "page contains non-frozen tuples but visibility map bit is set in relation \"%s\" page %u",
+ relname, blkno);
+			PageClearAllVisible(page);
+			MarkBufferDirty(buf);
+			visibilitymap_clear(onerel, blkno, vmbuffer);
+		}
+
+		/*
 		 * It's possible for the value returned by GetOldestXmin() to move
 		 * backwards, so it's not wrong for us to see tuples that appear to
 		 * not be visible to everyone yet, while PD_ALL_VISIBLE is already

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

[HACKERS] Header and comments describing routines in incorrect shape in visibilitymap.c

[Michael Paquier]

Hi all,

I just bumped into a couple of things in visibilitymap.c:
- visibilitymap_clear clears all bits of a visibility map, its header
comment mentions the contrary
- The header of visibilitymap.c mentions correctly "a bit" when
referring to setting them, but when clearing, it should say that all
bits are cleared.
- visibilitymap_set can set multiple bits
- visibilitymap_pin can be called to set up more than 1 bit.

This can be summed by the patch attached.
Regards,
-- 
Michael
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index b472d31..fa42498 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -11,10 +11,10 @@
  *	  src/backend/access/heap/visibilitymap.c
  *
  * INTERFACE ROUTINES
- *		visibilitymap_clear  - clear a bit in the visibility map
- *		visibilitymap_pin	 - pin a map page for setting a bit
+ *		visibilitymap_clear  - clear all bits in the visibility map
+ *		visibilitymap_pin	 - pin a map page for setting bit(s)
  *		visibilitymap_pin_ok - check whether correct map page is already pinned
- *		visibilitymap_set	 - set a bit in a previously pinned page
+ *		visibilitymap_set	 - set bit(s) in a previously pinned page
  *		visibilitymap_get_status - get status of bits
  *		visibilitymap_count  - count number of bits set in visibility map
  *		visibilitymap_truncate	- truncate the visibility map
@@ -34,7 +34,7 @@
  * might not be true.
  *
  * Clearing visibility map bits is not separately WAL-logged.  The callers
- * must make sure that whenever a bit is cleared, the bit is cleared on WAL
+ * must make sure that whenever bits are cleared, the bits are cleared on WAL
  * replay of the updating operation as well.
  *
  * When we *set* a visibility map during VACUUM, we must write WAL.  This may
@@ -78,8 +78,8 @@
  * When a bit is set, the LSN of the visibility map page is updated to make
  * sure that the visibility map update doesn't get written to disk before the
  * WAL record of the changes that made it possible to set the bit is flushed.
- * But when a bit is cleared, we don't have to do that because it's always
- * safe to clear a bit in the map from correctness point of view.
+ * But when bits are cleared, we don't have to do that because it's always
+ * safe to clear bits in the map from correctness point of view.
  *
  *-
  */
@@ -195,9 +195,9 @@ visibilitymap_clear(Relation rel, BlockNumber heapBlk, Buffer buf)
 }
 
 /*
- *	visibilitymap_pin - pin a map page for setting a bit
+ *	visibilitymap_pin - pin a map page for setting bit(s)
  *
- * Setting a bit in the visibility map is a two-phase operation. First, call
+ * Setting bit(s) in the visibility map is a two-phase operation. First, call
  * visibilitymap_pin, to pin the visibility map page containing the bit for
  * the heap page. Because that can require I/O to read the map page, you
  * shouldn't hold a lock on the heap page while doing that. Then, call

-- 
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] Postgres_fdw join pushdown - wrong results with whole-row reference

[Etsuro Fujita]

On 2016/07/02 0:32, Robert Haas wrote:

On Wed, Jun 29, 2016 at 1:38 AM, Ashutosh Bapat
 wrote:

On Tue, Jun 28, 2016 at 12:52 PM, Etsuro Fujita
 wrote:

Please find attached an updated version.



This looks good to me. Regression tests pass.



Committed.  Thanks to Etsuro Fujita for the patch and to Ashutosh for
the review.


Thanks, Robert and Ashutosh!

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] [CF2016-9] Allow spaces in working path on tap-tests

[Michael Paquier]

On Tue, Jul 5, 2016 at 6:02 PM, Kyotaro HORIGUCHI
 wrote:
> Agreed. Grep'ing "system" in the source tree, I see no more place
> where needs the same fix.

Same conclusion here. I have added this stuff to the official patch tracker:
https://commitfest.postgresql.org/10/663/
I can as well produce patches for back-branches, feel free to ping me
if necessary.
-- 
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] What happens if tuple queue is full

[Tatsuo Ishii]

Hari,

> The queue can gets full in the case where the leader is processing is slow
> than the worker is producing tuples. In those scenarios, the worker waits
> until the the queue gets empty to place the new satisfied record.The worker
> gets awaken whenever the leader finishes reading the data from queue.

Thanks for the quick response! That is useful information.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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

[HACKERS] Fix typo in jsonb.c

[Masahiko Sawada]

Hi all,

Attached patch fixes the typo in jsonb.c
Please find it.

Regards,

--
Masahiko Sawada
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 256ef80..ab46823 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -1849,7 +1849,7 @@ jsonb_object_agg_transfn(PG_FUNCTION_ARGS)
 	single_scalar = false;
 
 	/*
-	 * values can be anything, including structured and null, so we treate
+	 * values can be anything, including structured and null, so we treat
 	 * them as in json_agg_transfn, except that single scalars are always
 	 * pushed as WJB_VALUE items.
 	 */

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

Re: [HACKERS] What happens if tuple queue is full

[Haribabu Kommi]

On Wed, Jul 6, 2016 at 10:57 AM, Tatsuo Ishii  wrote:
> Hi,
>
> Does anybody know what will happen if tuple queue is full, which is
> used by the parallel leader and workers? In my understanding memory
> used for the queue is a dynamic shared memory and it could be full.

The queue can gets full in the case where the leader is processing is slow
than the worker is producing tuples. In those scenarios, the worker waits
until the the queue gets empty to place the new satisfied record.The worker
gets awaken whenever the leader finishes reading the data from queue.


Regards,
Hari Babu
Fujitsu Australia


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

[HACKERS] What happens if tuple queue is full

[Tatsuo Ishii]

Hi,

Does anybody know what will happen if tuple queue is full, which is
used by the parallel leader and workers? In my understanding memory
used for the queue is a dynamic shared memory and it could be full.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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 query and temp_file_limit

[Tom Lane]

Peter Geoghegan  writes:
> On Tue, Jul 5, 2016 at 12:00 PM, Robert Haas  wrote:
>> I think that it is not worth mentioning specifically for
>> temp_file_limit; to me that seems to be a hole with no bottom.  We'll
>> end up arguing about which GUCs should mention it specifically and
>> there will be no end to it.

> I don't think that you need it for any other GUC, so I really don't
> know why you're concerned about a slippery slope.

FWIW, I agree with Robert on this.  It seems just weird to call out
temp_file_limit specifically.  Also, I don't agree that that's the
only interesting per-process resource consumption; max_files_per_process
seems much more likely to cause trouble in practice.

Perhaps we could change the wording of temp_file_limit's description
from "space that a session can use" to "space that a process can use"
to help clarify this?

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] Parallel query and temp_file_limit

[Peter Geoghegan]

On Tue, Jul 5, 2016 at 12:58 PM, Tom Lane  wrote:
> Perhaps we could change the wording of temp_file_limit's description
> from "space that a session can use" to "space that a process can use"
> to help clarify this?

That's all that I was looking for, really.

-- 
Peter Geoghegan


-- 
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] can we optimize STACK_DEPTH_SLOP

[Tom Lane]

Robert Haas  writes:
> On Tue, Jul 5, 2016 at 11:54 AM, Tom Lane  wrote:
>> I'm pretty nervous about reducing that materially without any
>> investigation into how much of the slop we actually use.

> To me it seems like using anything based on stack_rlimit/2 is pretty
> risky for the reason that you state, but I also think that not being
> able to start the database at all on some platforms with small stacks
> is bad.

My point was that this is something we should investigate, not just
guess about.

I did some experimentation using the attached quick-kluge patch, which
(1) causes each exiting server process to report its actual ending stack
size, and (2) hacks the STACK_DEPTH_SLOP test so that you can set
max_stack_depth considerably higher than what rlimit(2) claims.
Unfortunately the way I did (1) only works on systems with pmap; I'm not
sure how to make it more portable.

My results on an x86_64 RHEL6 system were pretty interesting:

1. All but two of the regression test scripts have ending stack sizes
of 188K to 196K.  There is one outlier at 296K (most likely the regex
test, though I did not stop to confirm that) and then there's the
errors.sql test, which intentionally provokes a "stack too deep" failure
and will therefore consume approximately max_stack_depth stack if it can.

2. With the RHEL6 default "ulimit -s" setting of 10240kB, you actually
have to increase max_stack_depth to 12275kB before you get a crash in
errors.sql.  At the highest passing value, 12274kB, pmap says we end
with
  1 7ffc51f6e000  12284K rw---[ stack ]
which is just shy of 2MB more than the alleged limit.  I conclude that
at least in this kernel version, the kernel doesn't complain until your
stack would be 2MB *more* than the ulimit -s value.

That result also says that at least for that particular test, the
value of STACK_DEPTH_SLOP could be as little as 10K without a crash,
even without this surprising kernel forgiveness.  But of course that
test isn't really pushing the slop factor, since it's only compiling a
trivial expression at each recursion depth.

Given these results I definitely wouldn't have a problem with reducing
STACK_DEPTH_SLOP to 200K, and you could possibly talk me down to less.
On x86_64.  Other architectures might be more stack-hungry, though.
I'm particularly worried about IA64 --- I wonder if anyone can perform
these same experiments on that?

regards, tom lane

diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index cc36b80..7740120 100644
*** a/src/backend/storage/ipc/ipc.c
--- b/src/backend/storage/ipc/ipc.c
*** static int	on_proc_exit_index,
*** 98,106 
--- 98,113 
  void
  proc_exit(int code)
  {
+ 	char	sysbuf[256];
+ 
  	/* Clean up everything that must be cleaned up */
  	proc_exit_prepare(code);
  
+ 	/* report stack size to stderr */
+ 	snprintf(sysbuf, sizeof(sysbuf), "pmap %d | grep stack 1>&2",
+ 			 (int) getpid());
+ 	system(sysbuf);
+ 
  #ifdef PROFILE_PID_DIR
  	{
  		/*
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index 7254355..009bec2 100644
*** a/src/include/tcop/tcopprot.h
--- b/src/include/tcop/tcopprot.h
***
*** 27,33 
  
  
  /* Required daylight between max_stack_depth and the kernel limit, in bytes */
! #define STACK_DEPTH_SLOP (512 * 1024L)
  
  extern CommandDest whereToSendOutput;
  extern PGDLLIMPORT const char *debug_query_string;
--- 27,33 
  
  
  /* Required daylight between max_stack_depth and the kernel limit, in bytes */
! #define STACK_DEPTH_SLOP (-100 * 1024L * 1024L)
  
  extern CommandDest whereToSendOutput;
  extern PGDLLIMPORT const char *debug_query_string;

-- 
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] Typo Patch

[Alvaro Herrera]

CharSyam wrote:
> Yes, Some typos problems, It is hard to figure them out.
> But in this patch, guranteed is just misspelling of guaranteed.

I agree with Robert: the sentence as a whole makes no sense:

 * We need to check all possible distances, so reset Lpos
 * to guaranteed not yet satisfied position.

It's hard to justify changing a single character when what we should be
doing is rewriting the comment completely.

-- 
Á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] Typo Patch

[David G. Johnston]

On Tue, Jul 5, 2016 at 11:54 AM, Robert Haas  wrote:

> On Sat, Jul 2, 2016 at 12:17 PM, CharSyam  wrote:
> > I fixed typos. and attached patch for this.
> > Thanks.
> >
> > I only changed comments only in src/backend/utils/adt/tsvector_op.c
>
> Well, that's definitely a typo.  However, typo or no typo, it's hard
> for me to figure out with any certainty what that sentence actually
> means.
>
​​

Given that "curitem->qoperator.​distance" is constant.

Also, both loops go from low to high position (over the same scale)  with
the inner loop stopping as soon as it moves beyond the position of the
outer loop - namely that the left/inner item is always positioned to the
left of the right/outer operand within the document being search.

Regardless of whether there is a match at this meeting point increasing the
outer loop's position will not cause any of the previously checked (at the
lower positions) inner loop items to match where they did not before.  I
say this but I'm concerned that for sufficiently large values of
curitem->qoperator.distance a given left operand could match multiple right
operands since the distance is a maximum extent.

Thus, in the case of a match the current Lpos needs to be checked again
during the subsequent iterator of the outer loop.

The "else if (Rpos - Lpos < distance) break" block though is oddly
commented/written since distance is a maximum - shouldn't this be treated
as a match as well?

Since, for sufficiently large values of "distance", a single left operand
could cause multiple right operands to be matched when the less-than
condition matches we need to leave Lpos alone and try the next Rpos against
it.  For the greater than (default) and the equal cases we can skip
rechecking the current Lpos.

The comment in question can be removed - we're not actually resetting
anything here.  The use of LposStart is not really needed - just make sure
to leave Lpos in the correct position (i.e. optional increment on all
paths) and the inner while loop will do the correct thing.

It seems a bit odd to be keying off of the RIGHT operand and returning its
position when left-to-right languages would consider the position of the
leading word to be reported.

Code Comment Suggestion:

For each physically positioned right-side operand iterate over each
instance of the left-side operand to locate one within the specified
distance.  As soon as a match is found move onto the next right-operand and
continue searching starting with the last checked left-side operand.  Note
that for an exact distance match the current left-side operand can be
skipped over.

For some graphical imagery consider how a Slinky operates.  Hold the left
side, move the right side, release the left and let it collapse; repeat.
In this case, though, the collapsing can be stopped mid-stream since the
distance check has an inequality.

The following shouldn't be taken as an actual patch submission but rather a
codification of the above.

diff --git a/src/backend/utils/adt/tsvector_op.c
b/src/backend/utils/adt/tsvector_op.c
index 242b7e1..fefaca5 100644
--- a/src/backend/utils/adt/tsvector_op.c
+++ b/src/backend/utils/adt/tsvector_op.c
@@ -1375,7 +1375,6 @@ TS_phrase_execute(QueryItem *curitem,
  ExecPhraseData Ldata = {0, false, NULL},
  Rdata = {0, false, NULL};
  WordEntryPos *Lpos,
-   *LposStart,
*Rpos,
*pos_iter = NULL;

@@ -1423,15 +1422,17 @@ TS_phrase_execute(QueryItem *curitem,
  * ExecPhraseData->data can point to the tsvector's WordEntryPosVector
  */

+ /*
+  * For each physically positioned right-side operand iterate over each
+  * instance of the left-side operand to locate one within the specified
+  * distance.  As soon as a match is found move onto the next right-operand
+  * and continue searching starting with the last checked left-side
operand.
+  * Note that for an exact distance match the current left-side operand
+  * can be skipped over.
+  */
  Rpos = Rdata.pos;
- LposStart = Ldata.pos;
  while (Rpos < Rdata.pos + Rdata.npos)
  {
- /*
- * We need to check all possible distances, so reset Lpos
- * to guranteed not yet satisfied position.
- */
- Lpos = LposStart;
  while (Lpos < Ldata.pos + Ldata.npos)
  {
  if (WEP_GETPOS(*Rpos) - WEP_GETPOS(*Lpos) ==
@@ -1449,7 +1450,7 @@ TS_phrase_execute(QueryItem *curitem,
  * could not satisfy distance for any other right
  * position
  */
- LposStart = Lpos + 1;
+ Lpos++
  break;
  }
  else
@@ -1462,16 +1463,26 @@ TS_phrase_execute(QueryItem *curitem,
  }

  }
- else if (WEP_GETPOS(*Rpos) <= WEP_GETPOS(*Lpos) ||
- WEP_GETPOS(*Rpos) - WEP_GETPOS(*Lpos) <
+ else if (WEP_GETPOS(*Rpos) <= WEP_GETPOS(*Lpos)
+ {
+ /*
+ * No Increment - we are beyond the current right operand
+ * so its possible this left operand could match the
next right
+ * operand.
+ */
+ break;
+ }
+ else if (WEP_GETPOS(*Rpos) - WEP_GETPOS(*Lpos) <
  curitem->qoperator.distance)
  {
+ /* 

Re: [HACKERS] can we optimize STACK_DEPTH_SLOP

[Robert Haas]

On Tue, Jul 5, 2016 at 11:54 AM, Tom Lane  wrote:
> Greg Stark  writes:
>> Poking at NetBSD kernel source it looks like the default ulimit -s
>> depends on the architecture and ranges from 512k to 16M. Postgres
>> insists on max_stack_depth being STACK_DEPTH_SLOP -- ie 512kB -- less
>> than the ulimit setting making it impossible to start up on
>> architectures with a default of 512kB without raising the ulimit.
>
>> If we could just lower it to 384kB then Postgres would start up but I
>> wonder if we should just use MIN(stack_rlimit/2, STACK
>> _DEPTH_SLOP) so that there's always a setting of max_stack_depth that
>> would allow Postgres to start.
>
> I'm pretty nervous about reducing that materially without any
> investigation into how much of the slop we actually use.  Our assumption
> so far has generally been that only recursive routines need to have any
> stack depth check; but there are plenty of very deep non-recursive call
> paths.  I do not think we're doing people any favors by letting them skip
> fooling with "ulimit -s" if the result is that their database crashes
> under stress.  For that matter, even if we were sure we'd produce a
> "stack too deep" error rather than crashing, that's still not very nice
> if it happens on run-of-the-mill queries.

To me it seems like using anything based on stack_rlimit/2 is pretty
risky for the reason that you state, but I also think that not being
able to start the database at all on some platforms with small stacks
is bad.  If I had to guess, I'd bet that most functions in the backend
use a few hundred bytes of stack space or less, so that even 100kB of
stack space is enough for hundreds of stack frames.  If we're putting
that kind of depth on the stack without ever checking the stack depth,
we deserve what we get.  That having been said, it wouldn't surprise
me to find that we have functions here and there which put objects
that are many kB in size on the stack, making it much easier to
overrun the available stack space in only a few frames.  It would be
nice if there were a tool that you could run over your binaries and
have it dump out the names of all functions that create large stack
frames, but I don't know of one.

-- 
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 query and temp_file_limit

[Peter Geoghegan]

On Tue, Jul 5, 2016 at 12:00 PM, Robert Haas  wrote:
> I think that it is not worth mentioning specifically for
> temp_file_limit; to me that seems to be a hole with no bottom.  We'll
> end up arguing about which GUCs should mention it specifically and
> there will be no end to it.

I don't think that you need it for any other GUC, so I really don't
know why you're concerned about a slippery slope. The only other
resource GUC that is scoped per session that I can see is
temp_buffers, but that doesn't need to change, since parallel workers
cannot use temp_buffers directly in practice. max_files_per_process is
already clearly per process, so no change needed there either.

I don't see a case other than temp_file_limit that appears to be even
marginally in need of a specific note.

-- 
Peter Geoghegan


-- 
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 query and temp_file_limit

[Robert Haas]

On Tue, Jul 5, 2016 at 1:58 PM, Peter Geoghegan  wrote:
> On Tue, Jul 5, 2016 at 7:45 AM, Robert Haas  wrote:
>> Since Peter doesn't seem in a hurry to produce a patch for this issue,
>> I wrote one.  It is attached.  I'll commit this in a day or two if
>> nobody objects.
>
> Sorry about the delay.
>
> Your patch seems reasonable, but I thought we'd also want to change
> "per session" to "per session (with an additional temp_file_limit
> allowance within each parallel worker)" for temp_file_limit.
>
> I think it's worthwhile noting this for temp_file_limit specifically,
> since it's explicitly a per session limit, whereas users are quite
> used to the idea that work_mem might be doled out multiple times for
> multiple executor nodes.

I think that it is not worth mentioning specifically for
temp_file_limit; to me that seems to be a hole with no bottom.  We'll
end up arguing about which GUCs should mention it specifically and
there will be no end to it.

We can see what other people think, but that's my position.

-- 
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] reserved role names

[Andrew Gierth]

> "Joshua" == Joshua D Drake  writes:

 Joshua> Is it intentional that insert and delete are allowed and select
 Joshua> is not or is it an oversight?

Just an artifact of SELECT being fully reserved (needed for (SELECT ...)
syntax to work right) while INSERT and DELETE are unreserved.

-- 
Andrew (irc:RhodiumToad)


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

[HACKERS] reserved role names

[Joshua D. Drake]

Hey,

For grins while writing up an example on RLS for a client I tried the 
following:


policy=# create role insert;
CREATE ROLE

policy=# create role select;
ERROR:  syntax error at or near "select"
LINE 1: create role select;
^
policy=# create role delete;
CREATE ROLE

Is it intentional that insert and delete are allowed and select is not 
or is it an oversight?


Sincerely,

JD

--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
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 query and temp_file_limit

[Peter Geoghegan]

On Tue, Jul 5, 2016 at 7:45 AM, Robert Haas  wrote:
> Since Peter doesn't seem in a hurry to produce a patch for this issue,
> I wrote one.  It is attached.  I'll commit this in a day or two if
> nobody objects.

Sorry about the delay.

Your patch seems reasonable, but I thought we'd also want to change
"per session" to "per session (with an additional temp_file_limit
allowance within each parallel worker)" for temp_file_limit.

I think it's worthwhile noting this for temp_file_limit specifically,
since it's explicitly a per session limit, whereas users are quite
used to the idea that work_mem might be doled out multiple times for
multiple executor nodes.

-- 
Peter Geoghegan


-- 
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] Typo Patch

[CharSyam]

Yes, Some typos problems, It is hard to figure them out.
But in this patch, guranteed is just misspelling of guaranteed.

Thanks.

2016-07-06 0:54 GMT+09:00 Robert Haas :

> On Sat, Jul 2, 2016 at 12:17 PM, CharSyam  wrote:
> > I fixed typos. and attached patch for this.
> > Thanks.
> >
> > I only changed comments only in src/backend/utils/adt/tsvector_op.c
>
> Well, that's definitely a typo.  However, typo or no typo, it's hard
> for me to figure out with any certainty what that sentence actually
> means.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] Typo Patch

[Robert Haas]

On Sat, Jul 2, 2016 at 12:17 PM, CharSyam  wrote:
> I fixed typos. and attached patch for this.
> Thanks.
>
> I only changed comments only in src/backend/utils/adt/tsvector_op.c

Well, that's definitely a typo.  However, typo or no typo, it's hard
for me to figure out with any certainty what that sentence actually
means.

-- 
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] can we optimize STACK_DEPTH_SLOP

[Tom Lane]

Greg Stark  writes:
> Poking at NetBSD kernel source it looks like the default ulimit -s
> depends on the architecture and ranges from 512k to 16M. Postgres
> insists on max_stack_depth being STACK_DEPTH_SLOP -- ie 512kB -- less
> than the ulimit setting making it impossible to start up on
> architectures with a default of 512kB without raising the ulimit.

> If we could just lower it to 384kB then Postgres would start up but I
> wonder if we should just use MIN(stack_rlimit/2, STACK
> _DEPTH_SLOP) so that there's always a setting of max_stack_depth that
> would allow Postgres to start.

I'm pretty nervous about reducing that materially without any
investigation into how much of the slop we actually use.  Our assumption
so far has generally been that only recursive routines need to have any
stack depth check; but there are plenty of very deep non-recursive call
paths.  I do not think we're doing people any favors by letting them skip
fooling with "ulimit -s" if the result is that their database crashes
under stress.  For that matter, even if we were sure we'd produce a
"stack too deep" error rather than crashing, that's still not very nice
if it happens on run-of-the-mill queries.

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] asynchronous and vectorized execution

[Robert Haas]

On Wed, Jun 29, 2016 at 11:00 AM, Amit Khandekar  wrote:
> We may also want to consider handling abstract events such as
> "tuples-are-available-at-plan-node-X".
>
> One benefit is : we can combine this with batch processing. For e.g. in case
> of an Append node containing foreign scans, its parent node may not want to
> process the Append node result until Append is ready with at least 1000
> rows. In that case, Append node needs to raise an event
> "n-tuples-are-ready"; we cannot just rely on fd-ready events. fd-ready event
> will wake up the foreign scan, but it may not eventually cause its parent
> Append node to in turn wake up it's parent.

Right, I agree.  I think this case only arises in parallel query.  In
serial execution, there's not really any way for a plan node to just
become ready other than an FD or latch event or the parent becoming
ready.  But in parallel query it can happen, of course, because some
other backend can do work that makes that node ready to produce
tuples.

It's not necessarily the case that we have to deal with this in the
initial patches for this feature, because the most obvious win for
this sort of thing is when we have an Append of ForeignScan plans.
Sure, parallel query has interesting cases, too, but a prototype that
just handles Append over a bunch of postgres_fdw ForeignScans would be
pretty cool.  I suggest that we make that the initial goal here.

> How we can do this event abstraction is the other question. We can have one
> latch for each of the event, and each node would raise its own event by
> setting the corresponding latch. But I am not sure about latches within a
> single process as against one process waking up another process. Or else,
> some internal event structures needs to be present (in estate ?), which then
> ExecProcNode would use when it does the event looping to wake up (i.e.
> execute) required nodes.

I think adding more latches would be a bad idea.  What I think we
should do instead is add two additional data structures to dynamic
shared memory:

1. An array of PGPROC * pointers for all of the workers.  Processes
add their PGPROC * to this array as they start up.  Then, parallel.h
can expose new API ParallelWorkerSetLatchesForGroup(void).  In the
leader, this sets the latch for every worker process for every
parallel context with which the leader is associated; in a worker, it
sets the latch for other processes in the parallel group, and the
leader also.

2. An array of executor nodes where one process might do something
that allows other processes to make progress on that node.  This would
be set up somehow by execParallel.c, which would need to somehow
figure out which plan nodes want to be included in the list.  When an
executor node does something that might unblock other workers, it
calls ParallelWorkerSetLatchesForGroup() and the async stuff then
tries calling all of the nodes in this array again to see if any of
them now think that they've got tuples to return (or just to let them
do additional work without returning tuples).

> Also, the WaitEvent.user_data field can have some more info besides the plan
> state. It can have its parent PlanState stored, so that we don't have to
> have parent field in plan state. It also can have some more data such as
> "n-tuples-available".

I don't think that works, because execution may need to flow
arbitrarily far up the tree.  Just knowing the immediate parent isn't
good enough.  If it generates a tuple, then you have to in turn call
it's parent, and that one then produces a tuple, you have to continue
on even further up the tree.  I think it's going to be very awkward to
make this work without those parent pointers.

BTW, we also need to benchmark those changes to add the parent
pointers and change the return conventions and see if they have any
measurable impact on performance.

-- 
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] Documentation fixes for pg_visibility

[Robert Haas]

On Fri, Jul 1, 2016 at 10:20 PM, Amit Kapila  wrote:
 Right, something like that.  I think Andres actually wants something
 like this in 9.6, and I'm inclined to think it might be a good idea,
 too.  I think there should probably be a test for
 all_visible_according_to_vm at the beginning of that so that we don't
 add more visibility map checks for pages where we already know the VM
 bit can't possibly be set.

 Other opinions on the concept or the patch?
>>>
>>> +1 on the idea.
>>>
>>> + PageClearAllVisible(page);
>>> + MarkBufferDirty(buf);
>>>
>>> What is the need to clear the Page level bit, if it is already
>>> cleared, doesn't '!all_frozen' indicate that?
>>
>> No, I don't think so.  I think all_frozen indicates whether we think
>> that all tuples on the page qualify as fully frozen.  I don't think it
>> tells us anything about whether PD_ALL_VISIBLE is set on the page.
>
> Then, can we decide to clear it on that basis?  Isn't it possible that
> page is marked as all_visible, even if it contains frozen tuples?In
> the other nearby code (refer below part of code), we only clear the
> page level bit after ensuring it is set.  Am I missing something?
>
> else if (PageIsAllVisible(page) && has_dead_tuples)
> {
> elog(WARNING, "page containing dead tuples is marked as all-visible in
> relation \"%s\" page %u",
> relname, blkno);
> PageClearAllVisible(page);
> MarkBufferDirty(buf);
> visibilitymap_clear(onerel, blkno, vmbuffer);
> }

So I'm a bit confused about what you are saying here.  If the page is
marked all-frozen but actually isn't all-frozen, then we should clear
the all-frozen bit in the VM.  The easiest way to do that is to clear
both bits in the VM plus the page-level bit, as done here, because we
don't presently have a way of clearing just one of the visibility map
bits.

Now, since the heap_lock_tuple issue requires us to introduce a new
method to clear all-visible without clearing all-frozen, we could
possibly use that here too, once we have 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] Parallel query and temp_file_limit

[Robert Haas]

On Tue, Jun 21, 2016 at 8:15 AM, Robert Haas  wrote:
> On Mon, Jun 20, 2016 at 11:01 PM, Tom Lane  wrote:
>> Peter Geoghegan  writes:
>>> On Wed, May 18, 2016 at 3:40 AM, Robert Haas  wrote:
 What I'm tempted to do is trying to document that, as a point of
 policy, parallel query in 9.6 uses up to (workers + 1) times the
 resources that a single session might use.  That includes not only CPU
 but also things like work_mem and temp file space.  This obviously
 isn't ideal, but it's what could be done by the ship date.
>>
>>> Where would that be documented, though? Would it need to be noted in
>>> the case of each such GUC?
>>
>> Why can't we just note this in the number-of-workers GUCs?  It's not like
>> there even *is* a GUC for many of our per-process resource consumption
>> behaviors.
>
> +1.

Since Peter doesn't seem in a hurry to produce a patch for this issue,
I wrote one.  It is attached.  I'll commit this in a day or two if
nobody objects.

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


parallel-workers-guc-doc.patch
Description: invalid/octet-stream

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

Re: [HACKERS] Reviewing freeze map code

[Masahiko Sawada]

On Mon, Jul 4, 2016 at 5:44 PM, Masahiko Sawada  wrote:
> On Sat, Jul 2, 2016 at 12:34 PM, Amit Kapila  wrote:
>> On Fri, Jul 1, 2016 at 7:52 PM, Masahiko Sawada  
>> wrote:
>>> On Fri, Jul 1, 2016 at 11:12 AM, Amit Kapila  
>>> wrote:

 Why do you think IndexOnlyScan will return wrong result?  If the
 server crash in the way as you described, the transaction that has
 made modifications will anyway be considered aborted, so the result of
 IndexOnlyScan should not be wrong.

>>>
>>> Ah, you're right, I misunderstood.
>>>
>>> Attached updated patch incorporating your comments.
>>> I've changed it so that heap_xlog_lock clears vm flags if page is
>>> marked all frozen.
>>>
>>
>> I think we should make a similar change in heap_lock_tuple API as
>> well.
>> Also, currently by default heap_xlog_lock tuple tries to clear
>> the visibility flags, isn't it better to handle it as we do at all
>> other places (ex. see log_heap_update), by logging the information
>> about same.
>
> I will deal with them.
>
>> Though, it is important to get the patch right, but I feel in the
>> meantime, it might be better to start benchmarking.  AFAIU, even if
>> change some part of information while WAL logging it, the benchmark
>> results won't be much different.
>
> Okay, I will do the benchmark test as well.
>

I measured the thoughput and the output quantity of WAL with HEAD and
HEAD+patch(attached) on my virtual environment.
I used pgbench with attached custom script file which sets 3200 length
string to the filler column in order to make toast data.
The scale factor is 1000 and pgbench options are, -c 4 -T 600 -f toast_test.sql.
After changed filler column to the text data type I ran it.

* Throughput
HEAD : 1833.204172
Patched : 1827.399482

* Output quantity of WAL
HEAD :  7771 MB
Patched : 8082 MB

The throughput is almost same, but the ouput quantity of WAL is
slightly increased. (about 4%)

Regards,

--
Masahiko Sawada


toast_test.sql
Description: Binary data
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 57da57a..fd66527 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3923,6 +3923,17 @@ l2:
 
 	if (need_toast || newtupsize > pagefree)
 	{
+		/*
+		 * To prevent data corruption due to updating old tuple by
+		 * other backends after released buffer, we need to emit that
+		 * xmax of old tuple is set and clear visibility map bits if
+		 * needed before releasing buffer. We can reuse xl_heap_lock
+		 * for this purpose. It should be fine even if we crash midway
+		 * from this section and the actual updating one later, since
+		 * the xmax will appear to come from an aborted xid.
+		 */
+		START_CRIT_SECTION();
+
 		/* Clear obsolete visibility flags ... */
 		oldtup.t_data->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
 		oldtup.t_data->t_infomask2 &= ~HEAP_KEYS_UPDATED;
@@ -3936,6 +3947,46 @@ l2:
 		/* temporarily make it look not-updated */
 		oldtup.t_data->t_ctid = oldtup.t_self;
 		already_marked = true;
+
+		/* Clear PD_ALL_VISIBLE flags */
+		if (PageIsAllVisible(BufferGetPage(buffer)))
+		{
+			all_visible_cleared = true;
+			PageClearAllVisible(BufferGetPage(buffer));
+			visibilitymap_clear(relation, BufferGetBlockNumber(buffer),
+vmbuffer);
+		}
+
+		MarkBufferDirty(buffer);
+
+		if (RelationNeedsWAL(relation))
+		{
+			xl_heap_lock xlrec;
+			XLogRecPtr recptr;
+
+			/*
+			 * For logical decoding we need combocids to properly decode the
+			 * catalog.
+			 */
+			if (RelationIsAccessibleInLogicalDecoding(relation))
+log_heap_new_cid(relation, );
+
+			XLogBeginInsert();
+			XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+
+			xlrec.offnum = ItemPointerGetOffsetNumber(_self);
+			xlrec.locking_xid = xmax_old_tuple;
+			xlrec.infobits_set = compute_infobits(oldtup.t_data->t_infomask,
+  oldtup.t_data->t_infomask2);
+			if (all_visible_cleared)
+xlrec.infobits_set |= XLHL_ALL_VISIBLE_CLEARED;
+			XLogRegisterData((char *) , SizeOfHeapLock);
+			recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_LOCK);
+			PageSetLSN(page, recptr);
+		}
+
+		END_CRIT_SECTION();
+
 		LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 
 		/*
@@ -4140,7 +4191,8 @@ l2:
 		 */
 		if (RelationIsAccessibleInLogicalDecoding(relation))
 		{
-			log_heap_new_cid(relation, );
+			if (!already_marked)
+log_heap_new_cid(relation, );
 			log_heap_new_cid(relation, heaptup);
 		}
 
@@ -4513,6 +4565,7 @@ heap_lock_tuple(Relation relation, HeapTuple tuple,
 new_infomask2;
 	bool		first_time = true;
 	bool		have_tuple_lock = false;
+	bool		all_visible_cleared = false;
 
 	*buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(tid));
 	LockBuffer(*buffer, BUFFER_LOCK_EXCLUSIVE);
@@ -5034,6 +5087,18 @@ failed:
 	if (HEAP_XMAX_IS_LOCKED_ONLY(new_infomask))
 		tuple->t_data->t_ctid = *tid;
 
+	/* Clear PD_ALL_VISIBLE flags */
+	if 

Re: [HACKERS] fixing subplan/subquery confusion

[Robert Haas]

On Sun, Jul 3, 2016 at 6:46 PM, Tom Lane  wrote:
> I wrote:
>> Robert Haas  writes:
>>> On Sun, Jul 3, 2016 at 1:14 PM, Tom Lane  wrote:
 You mentioned that you'll be on vacation for much of July.  If you like,
 I will take this open item off your hands, since I'll be around and can
 deal with any bugs that pop up in it.
>
>>> That would be much appreciated.
>
>> OK, will do.
>
> I've reviewed, adjusted, and pushed this patch, and accordingly am
> marking the open item closed.  (Any remaining bugs should become
> new open items.)

Thanks.  Looks good to me.

-- 
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] OpenSSL 1.1 breaks configure and more

[Andreas Karlsson]

On 07/05/2016 11:13 AM, Victor Wagner wrote:

On Fri, 1 Jul 2016 02:27:03 +0200
Andreas Karlsson  wrote:

0003-Remove-OpenSSL-1.1-deprecation-warnings.patch

Silence all warnings. This commit changes more things and is not
necessary for getting PostgreSQL to build against 1.1.


This patch breaks feature, which exists in PostgreSQL since 8.2 -
support for SSL ciphers, provided by loadable modules such as Russian
national standard (GOST) algorithms, and support for cryptographic
hardware tokens (which are also supported by loadble modules called
engines in OpenSSL).

Call for OPENSSL_config was added to PostgreSQL for this purpose - it
loads default OpenSSL configuration file, where such things as crypto
hardware modules can be configured.

If we wish to keep this functionality, we need to explicitely call

OPENSSL_init_ssl(INIT_LOAD_CONFIG,NULL) instead of deprecated
OPENSSL_config in 1.1.0


Thanks for testing the patches!

I have attached a new set of patches which this is fixed. I have also 
skipped the last patch since OpenSSL has fixed the two issues I have 
mentioned earlier in this thread.


Andreas
>From 0e6d78ad199bde8c4e698952a3b5e4375371f072 Mon Sep 17 00:00:00 2001
From: Andreas Karlsson 
Date: Tue, 28 Jun 2016 05:55:03 +0200
Subject: [PATCH 1/3] Fixes for compiling with OpenSSL 1.1

- Check for SSL_new now that SSL_library_init is a macro
- Do not access struct members directly
- RAND_SSLeay was renamed to RAND_OpenSSL()
---
 configure| 44 
 configure.in |  4 +--
 contrib/pgcrypto/openssl.c   | 28 
 contrib/sslinfo/sslinfo.c| 14 ++
 src/backend/libpq/be-secure-openssl.c| 39 +++-
 src/interfaces/libpq/fe-secure-openssl.c | 39 +++-
 6 files changed, 98 insertions(+), 70 deletions(-)

diff --git a/configure b/configure
index 1f42c8a..ca83738 100755
--- a/configure
+++ b/configure
@@ -9538,9 +9538,9 @@ else
   as_fn_error $? "library 'crypto' is required for OpenSSL" "$LINENO" 5
 fi
 
- { $as_echo "$as_me:${as_lineno-$LINENO}: checking for SSL_library_init in -lssl" >&5
-$as_echo_n "checking for SSL_library_init in -lssl... " >&6; }
-if ${ac_cv_lib_ssl_SSL_library_init+:} false; then :
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for SSL_new in -lssl" >&5
+$as_echo_n "checking for SSL_new in -lssl... " >&6; }
+if ${ac_cv_lib_ssl_SSL_new+:} false; then :
   $as_echo_n "(cached) " >&6
 else
   ac_check_lib_save_LIBS=$LIBS
@@ -9554,27 +9554,27 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 #ifdef __cplusplus
 extern "C"
 #endif
-char SSL_library_init ();
+char SSL_new ();
 int
 main ()
 {
-return SSL_library_init ();
+return SSL_new ();
   ;
   return 0;
 }
 _ACEOF
 if ac_fn_c_try_link "$LINENO"; then :
-  ac_cv_lib_ssl_SSL_library_init=yes
+  ac_cv_lib_ssl_SSL_new=yes
 else
-  ac_cv_lib_ssl_SSL_library_init=no
+  ac_cv_lib_ssl_SSL_new=no
 fi
 rm -f core conftest.err conftest.$ac_objext \
 conftest$ac_exeext conftest.$ac_ext
 LIBS=$ac_check_lib_save_LIBS
 fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_ssl_SSL_library_init" >&5
-$as_echo "$ac_cv_lib_ssl_SSL_library_init" >&6; }
-if test "x$ac_cv_lib_ssl_SSL_library_init" = xyes; then :
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_ssl_SSL_new" >&5
+$as_echo "$ac_cv_lib_ssl_SSL_new" >&6; }
+if test "x$ac_cv_lib_ssl_SSL_new" = xyes; then :
   cat >>confdefs.h <<_ACEOF
 #define HAVE_LIBSSL 1
 _ACEOF
@@ -9644,9 +9644,9 @@ else
   as_fn_error $? "library 'eay32' or 'crypto' is required for OpenSSL" "$LINENO" 5
 fi
 
- { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing SSL_library_init" >&5
-$as_echo_n "checking for library containing SSL_library_init... " >&6; }
-if ${ac_cv_search_SSL_library_init+:} false; then :
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing SSL_new" >&5
+$as_echo_n "checking for library containing SSL_new... " >&6; }
+if ${ac_cv_search_SSL_new+:} false; then :
   $as_echo_n "(cached) " >&6
 else
   ac_func_search_save_LIBS=$LIBS
@@ -9659,11 +9659,11 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 #ifdef __cplusplus
 extern "C"
 #endif
-char SSL_library_init ();
+char SSL_new ();
 int
 main ()
 {
-return SSL_library_init ();
+return SSL_new ();
   ;
   return 0;
 }
@@ -9676,25 +9676,25 @@ for ac_lib in '' ssleay32 ssl; do
 LIBS="-l$ac_lib  $ac_func_search_save_LIBS"
   fi
   if ac_fn_c_try_link "$LINENO"; then :
-  ac_cv_search_SSL_library_init=$ac_res
+  ac_cv_search_SSL_new=$ac_res
 fi
 rm -f core conftest.err conftest.$ac_objext \
 conftest$ac_exeext
-  if ${ac_cv_search_SSL_library_init+:} false; then :
+  if ${ac_cv_search_SSL_new+:} false; then :
   break
 fi
 done
-if ${ac_cv_search_SSL_library_init+:} false; then :
+if ${ac_cv_search_SSL_new+:} false; then :
 
 else
-  

[HACKERS] can we optimize STACK_DEPTH_SLOP

[Greg Stark]

Poking at NetBSD kernel source it looks like the default ulimit -s
depends on the architecture and ranges from 512k to 16M. Postgres
insists on max_stack_depth being STACK_DEPTH_SLOP -- ie 512kB -- less
than the ulimit setting making it impossible to start up on
architectures with a default of 512kB without raising the ulimit.

If we could just lower it to 384kB then Postgres would start up but I
wonder if we should just use MIN(stack_rlimit/2, STACK
_DEPTH_SLOP) so that there's always a setting of max_stack_depth that
would allow Postgres to start.

./arch/sun2/include/vmparam.h:73:#define DFLSSIZ (512*1024) /* initial
stack size limit */
./arch/arm/include/arm32/vmparam.h:66:#define DFLSSIZ (2*1024*1024) /*
initial stack size limit */
./arch/sun3/include/vmparam3.h:109:#define DFLSSIZ (512*1024) /*
initial stack size limit */
./arch/sun3/include/vmparam3x.h:58:#define DFLSSIZ (2*1024*1024) /*
initial stack size limit */
./arch/luna68k/include/vmparam.h:70:#define DFLSSIZ (512*1024) /*
initial stack size limit */
./arch/hppa/include/vmparam.h:62:#define DFLSSIZ (2*1024*1024) /*
initial stack size limit */
./arch/hp300/include/vmparam.h:82:#define DFLSSIZ (2*1024*1024) /*
initial stack size limit */
./arch/alpha/include/vmparam.h:79:#define DFLSSIZ (1<<21) /* initial
stack size (2M) */
./arch/acorn26/include/vmparam.h:55:#define DFLSSIZ (512*1024) /*
initial stack size limit */
./arch/amd64/include/vmparam.h:83:#define DFLSSIZ (4*1024*1024) /*
initial stack size limit */
./arch/amd64/include/vmparam.h:101:#define DFLSSIZ32 (2*1024*1024) /*
initial stack size limit */
./arch/ia64/include/vmparam.h:57:#define DFLSSIZ (1<<21) /* initial
stack size (2M) */
./arch/mvme68k/include/vmparam.h:82:#define DFLSSIZ (512*1024) /*
initial stack size limit */
./arch/i386/include/vmparam.h:74:#define DFLSSIZ (2*1024*1024) /*
initial stack size limit */
./arch/amiga/include/vmparam.h:82:#define DFLSSIZ (2*1024*1024) /*
initial stack size limit */
./arch/sparc/include/vmparam.h:94:#define DFLSSIZ (8*1024*1024) /*
initial stack size limit */
./arch/mips/include/vmparam.h:95:#define DFLSSIZ (4*1024*1024) /*
initial stack size limit */
./arch/mips/include/vmparam.h:114:#define DFLSSIZ (16*1024*1024) /*
initial stack size limit */
./arch/mips/include/vmparam.h:134:#define DFLSSIZ32 DFLTSIZ /* initial
stack size limit */
./arch/sh3/include/vmparam.h:69:#define DFLSSIZ (2 * 1024 * 1024)
./arch/mac68k/include/vmparam.h:115:#define DFLSSIZ (2*1024*1024) /*
initial stack size limit */
./arch/next68k/include/vmparam.h:89:#define DFLSSIZ (512*1024) /*
initial stack size limit */
./arch/news68k/include/vmparam.h:82:#define DFLSSIZ (512*1024) /*
initial stack size limit */
./arch/x68k/include/vmparam.h:74:#define DFLSSIZ (512*1024) /* initial
stack size limit */
./arch/cesfic/include/vmparam.h:82:#define DFLSSIZ (512*1024) /*
initial stack size limit */
./arch/usermode/include/vmparam.h:69:#define DFLSSIZ (2 * 1024 * 1024)
./arch/usermode/include/vmparam.h:78:#define DFLSSIZ (4 * 1024 * 1024)
./arch/powerpc/include/oea/vmparam.h:74:#define DFLSSIZ (2*1024*1024)
/* default stack size */
./arch/powerpc/include/ibm4xx/vmparam.h:60:#define DFLSSIZ
(2*1024*1024) /* default stack size */
./arch/powerpc/include/booke/vmparam.h:75:#define DFLSSIZ
(2*1024*1024) /* default stack size */
./arch/vax/include/vmparam.h:74:#define DFLSSIZ (512*1024) /* initial
stack size limit */
./arch/sparc64/include/vmparam.h:100:#define DFLSSIZ (2*1024*1024) /*
initial stack size limit */
./arch/sparc64/include/vmparam.h:125:#define DFLSSIZ (2*1024*1024) /*
initial stack size limit */
./arch/sparc64/include/vmparam.h:145:#define DFLSSIZ32 (2*1024*1024)
/* initial stack size limit */
./arch/atari/include/vmparam.h:81:#define DFLSSIZ (2*1024*1024) /*
initial stack size limit */


-- 
greg


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

Re: [HACKERS] Re: GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC]

[Andrew Borodin]

Here is a patch with updated WAL behavior.

I'm not certain about MAXALIGN for size of appended tuple. Currently
if anyone calls PageIndexTupleOverwrite() with unalligned tuple it
will ereport(ERROR).
I suspect that it should allign size instead.

Also I suspect that PageIndexTupleOverwrite() should make a call to
ItemIdSetNormal() to pretend it is generic function and not just a
private part of GiST.

Best regards, Andrey Borodin, Octonica & Ural Federal University.

2016-07-04 22:59 GMT+05:00 Andrew Borodin :
> Thank you, Alvaro.
>
> Yes, now I see. I'll update gistRedoPageUpdateRecord() to work
> accordingly with patched gistplacetopage().
> Also, i've noticed that PageAddItem uses MAXALIGN() macro to calculate
> tuple size. So, PageIndexTupleOverwrite should behave the same.
>
> About PageIndexDeleteNoCompact() in BRIN. I do not see why
> PageIndexDeleteNoCompact() is not a good solution instead of
> PageIndexTupleOverwrite? Will it mark tuple as unused until next
> PageAddItem will use it's place?
>
> Best regards, Andrey Borodin, Octonica & Ural Federal University.
>
> 2016-07-04 22:16 GMT+05:00 Alvaro Herrera :
>> Andrew Borodin wrote:
>>> Thank you, Amit.
>>>
>>> Currently, if WAL reconstruct page in another order it won't be a problem.
>>
>> We require that replay of WAL leads to identical bytes than the
>> original.  Among other things, this enables use of a tool that verifies
>> that WAL is working correctly simply by comparing page images.  So
>> please fix it instead of just verifying that this works for GiST.
>>
>> By the way, BRIN indexes have a need of this operation too.  The current
>> approach is to call PageIndexDeleteNoCompact followed by PageAddItem.
>> I suppose it would be beneficial to use your new routine there too.
>>
>> --
>> Álvaro Herrerahttp://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


PageIndexTupleOverwrite v3.patch
Description: Binary data

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

Re: [HACKERS] Cluster on NAS and data center.

[Krzysztof Kaczkowski]

Hello,

Thanks to emails, we have achieved what we wanted. This is what we’ve done:

Compilation (for 32 bit version, initdb with locale=C):

CFLAGS="-mx32 -fexcess-precision=standard -O2"
CXXFLAGS="-mx32"

configure --without-zlib --disable-float8-byval --without-readline
--host=x86_64-linux-gnux32

We also have installed libx32.

Right now we received cluster that is fully manageable from both systems.
Anyone see something dangerous with this compilation?

Re: [HACKERS] to_date_valid()

[David G. Johnston]

On Tue, Jul 5, 2016 at 5:22 AM, Andreas 'ads' Scherbaum <
adsm...@wars-nicht.de> wrote:

> On 05.07.2016 04:33, David G. Johnston wrote:
>
>> On Mon, Jul 4, 2016 at 8:39 PM, Andreas 'ads' Scherbaum
>> >wrote:
>>
>> On 04.07.2016 18:37, Pavel Stehule wrote:
>>
>>
>> I don't know if the name "strict" is best, but the name
>> "validate" is
>> not good too. Current to_date does some validations too.
>>
>>
>> Obviously not enough, because it allows invalid dates. I'd say that
>> the current to_date() merely validates the input format for string
>> parsing, and that the date is in range. But there is not much
>> validation on the date itself.
>>
>> So the name can't be "strict" because of the conflict with "NULL"
>> handling, and you don't like "valid" - what other options do you
>> offer?
>>
>>
>> ​We don't have to change the name...we could do something like how
>> RegularExpressions work - like (?i) - and just add  a new modifier ​code.
>>
>> ​'~-MI-DD' --that's a leading tilde, could be anything - even
>> something like "HM-MI-DD" for "historical mode"
>>
>
> Where to_timestamp() already uses HH for the hour? If you add another "H",
> that surely is confusing.


​I don't really try to pick final names for things until the idea has taken
hold...
​


>
>
>
> It seems that fixing it is back on the table, possibly even for 9.6
>> since this is such a hideous bug - one that closely resembles a cockroach
>> ;)
>>
>
> 9.6 is already in Beta, people are testing their applications against it.
> This would be a huge break, plus an API change - something you don't add in
> a Beta.
>
>
​Surely these beta testers would test against the RC before putting it into
production so I don't see an issue.  I tend to agree generally but the
point of beta is to find bugs and solicit suggestions for improvement to
features.  Having found a bug it doesn't make sense to avoid patching our
current unstable release.  This applies moreso because of our annual
release cycle.  On the topic of whether this becomes an exception to the
rule I'm largely ambivalent.

David J.

Re: [HACKERS] to_date_valid()

[Andreas 'ads' Scherbaum]

On 05.07.2016 04:33, David G. Johnston wrote:

On Mon, Jul 4, 2016 at 8:39 PM, Andreas 'ads' Scherbaum
>wrote:

On 04.07.2016 18:37, Pavel Stehule wrote:


I don't know if the name "strict" is best, but the name
"validate" is
not good too. Current to_date does some validations too.


Obviously not enough, because it allows invalid dates. I'd say that
the current to_date() merely validates the input format for string
parsing, and that the date is in range. But there is not much
validation on the date itself.

So the name can't be "strict" because of the conflict with "NULL"
handling, and you don't like "valid" - what other options do you offer?


​We don't have to change the name...we could do something like how
RegularExpressions work - like (?i) - and just add  a new modifier ​code.

​'~-MI-DD' --that's a leading tilde, could be anything - even
something like "HM-MI-DD" for "historical mode"


Where to_timestamp() already uses HH for the hour? If you add another 
"H", that surely is confusing.




It seems that fixing it is back on the table, possibly even for 9.6
since this is such a hideous bug - one that closely resembles a cockroach ;)


9.6 is already in Beta, people are testing their applications against 
it. This would be a huge break, plus an API change - something you don't 
add in a Beta.


--
Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project



--
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] to_date_valid()

[Andreas 'ads' Scherbaum]

On 05.07.2016 06:05, Pavel Stehule wrote:



2016-07-05 2:39 GMT+02:00 Andreas 'ads' Scherbaum >:

On 04.07.2016 18:37, Pavel Stehule wrote:


I don't know if the name "strict" is best, but the name
"validate" is
not good too. Current to_date does some validations too.


Obviously not enough, because it allows invalid dates. I'd say that
the current to_date() merely validates the input format for string
parsing, and that the date is in range. But there is not much
validation on the date itself.

So the name can't be "strict" because of the conflict with "NULL"
handling, and you don't like "valid" - what other options do you offer?


I have not - so third option looks best for me - it can be long name
"only_correct_date", "only_valid_date", "only_valid_date_on_input" ...


Then you don't have "to_date" in the function name, but still use 
"valid" in the name. How is that useful to remember the function? Where 
"to_date_valid" already gives you the idea that it is "to_date" with an 
additional "valid"ator.


Don't make it overly complicated.

--
Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project


--
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] OpenSSL 1.1 breaks configure and more

[Victor Wagner]

On Fri, 1 Jul 2016 02:27:03 +0200
Andreas Karlsson  wrote:


> 0003-Remove-OpenSSL-1.1-deprecation-warnings.patch
> 
> Silence all warnings. This commit changes more things and is not 
> necessary for getting PostgreSQL to build against 1.1.

This patch breaks feature, which exists in PostgreSQL since 8.2 -
support for SSL ciphers, provided by loadable modules such as Russian
national standard (GOST) algorithms, and support for cryptographic
hardware tokens (which are also supported by loadble modules called
engines in OpenSSL).

Call for OPENSSL_config was added to PostgreSQL for this purpose - it
loads default OpenSSL configuration file, where such things as crypto
hardware modules can be configured.

If we wish to keep this functionality, we need to explicitely call

OPENSSL_init_ssl(INIT_LOAD_CONFIG,NULL) instead of deprecated
OPENSSL_config in 1.1.0








-- 
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] [CF2016-9] Allow spaces in working path on tap-tests

[Kyotaro HORIGUCHI]

Hello,

At Tue, 5 Jul 2016 13:44:08 +0900, Michael Paquier  
wrote in 
> On Mon, Jul 4, 2016 at 4:44 PM, Michael Paquier
>  wrote:
> > And as is the command built for zic.exe in Install.pm, no? $target is
> > normally an absolute path per the call of Install().
> 
> Attached is the patch I have in mind. After more investigation zic.exe
> is indeed broken, $target can be a full path, and if it contains a
> space things blow up. The commands of vcregress upgradecheck need a
> cleanup as well. I have merged both patches together and the part for
> src/tools/msvc needs a backpatch. Separating both things is trivial
> anyway as the MSVC and the TAP stuff are on two completely separate
> paths.

Agreed. Grep'ing "system" in the source tree, I see no more place
where needs the same fix.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Magnus Hagander]

On Tue, Jul 5, 2016 at 10:06 AM, Michael Paquier 
wrote:

> On Mon, Jul 4, 2016 at 12:54 PM, Michael Paquier
>  wrote:
> > As I am coming back into that, I would as well suggest do the
> > following, that the current set of patches is clearly missing:
> > - Put the HMAC infrastructure stuff of pgcrypto into src/common/. It
> > is a bit a shame to not reuse what is currently available, then I
> > would suggest to reuse that with HMAC_SCRAM_SHAXXX as label.
> > - Move *all* the SHA-related things of pgcrypto to src/common,
> > including SHA1, SHA224 and SHA256. px_memset is a simple wrapper on
> > top of memset, we should clean up that first.
> > Any other things to consider that I am forgetting?
>
> After looking more into that, I have come up with PG-like equivalents
> of things in openssl/sha.h:
> pg_shaXX_init(pg_shaXX_ctx *ctx, data);
> pg_shaXX_update(pg_shaXX_ctx *ctx, uint8 *data, size_t len);
> pg_shaXX_final(uint8 *dest, pg_shaXX_ctx *ctx);
> Then think about shaXX as 1, 224, 256, 384 and 512.
>
> Hence all those functions, moved to src/common, finish with the
> following shape, take an init() one:
> #ifdef USE_SSL
> #define 
> #endif
> void
> pg_shaXX_init(pg_shaXX_ctx *ctx)
> {
> #ifdef USE_SSL
> SHAXX_Init((SHAXX_CTX *) ctx);
> #else
> //Here does the OpenBSD stuff, now part of pgcrypto
> #endif
> }
>
> And that's really ugly, all the OpenBSD things that are used by
> pgcrypto when the code is not built with --with-openssl gather into a
> single place with parts wrapped around USE_SSL. A less ugly solution
> would be to split that into two files, and one or the other gets
> included in OBJS depending on if the build is done with or without
> OpenSSL. We do a rather similar thing with fe/be-secure-openssl.c.
>

FWIW, the main reason for be-secure-openssl.c is that we could have support
for another external SSL library. The idea was never to have a builtin
replacement for it :)

However, is there something that's fundamentally better with the OpenSSL
implementation? Or should we just keep *just* the #else branch in the code,
the part we've imported from OpenBSD?

TLS is complex, we don't want to do that in that case. But just the sha
functions isn't *that* complex, is it?



> Another possibility is that we could say that SCRAM is designed to
> work with TLS, as mentioned a bit upthread via the RFC, so we would
> not support it in builds compiled without OpenSSL. I think that would
> be a shame, but it would simplify all this refactoring juggling.
>
> So, 3 possibilities here:
> 1) Use a single file src/common/sha.c that includes a set of functions
> using USE_SSL
> 2) Have two files in src/common, one when build is used with OpenSSL,
> and the second one when built-in methods are used
> 3) Disable the use of SCRAM when OpenSSL is not present in the build.
>
> Opinions? My heart goes for 2) because 1) is ugly, and 3) is not
> appealing in terms of flexibility.
>
>
I really dislike #3 - we want everybody to start using this...

I'm not sure how common a build without openssl is in the real world
though. RPMs, DEBs, Windows installers etc all build with OpenSSL. But we
probably don't want to make it mandatory, no...

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

Re: [HACKERS] to_date_valid()

[Albe Laurenz]

Andreas Karlsson wrote:
> On 07/04/2016 10:55 PM, Pavel Stehule wrote:
>> 2016-07-04 22:15 GMT+02:00 Andreas Karlsson wrote:
>>> I do not see a clear conclusion in the linked threads. For example
>>> Bruce calls it a bug in one of the emails
>>> (https://www.postgresql.org/message-id/201107200103.p6K13ix10517%40momjian.us).
>>>
>>> I think we should fix to_date() to throw an error. Personally I
>>> would be happy if my code broke due to this kind of change since the
>>> exception would reveal an old bug which has been there a long time
>>> eating my data. I cannot see a case where I would have wanted the
>>> current behavior.
>>
>> If I remember, this implementation is based on Oracle's behave.
> 
> In the thread I linked above they claim that Oracle (at least 10g) does
> not work like this.
[...]
> I do not have access to an Oracle installation so I cannot confirm this
> myself.

Oracle 12.1:

SQL> SELECT to_date('2016-12-40','-MM-DD') FROM dual;
SELECT to_date('2016-12-40','-MM-DD') FROM dual
   *
ERROR at line 1:
ORA-01847: day of month must be between 1 and last day of month

SQL> SELECT to_date('2017-02-29','-MM-DD') FROM dual;
SELECT to_date('2017-02-29','-MM-DD') FROM dual
   *
ERROR at line 1:
ORA-01839: date not valid for month specified

So no, compatibility with Oracle is certainly not the reason
to leave it as it is.

But notwithstanding your feeling that you would like your application
to break if it makes use of this behaviour, it is a change that might
make some people pretty unhappy - nobody can tell how many.

Yours,
Laurenz Albe

-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Mon, Jul 4, 2016 at 12:54 PM, Michael Paquier
 wrote:
> As I am coming back into that, I would as well suggest do the
> following, that the current set of patches is clearly missing:
> - Put the HMAC infrastructure stuff of pgcrypto into src/common/. It
> is a bit a shame to not reuse what is currently available, then I
> would suggest to reuse that with HMAC_SCRAM_SHAXXX as label.
> - Move *all* the SHA-related things of pgcrypto to src/common,
> including SHA1, SHA224 and SHA256. px_memset is a simple wrapper on
> top of memset, we should clean up that first.
> Any other things to consider that I am forgetting?

After looking more into that, I have come up with PG-like equivalents
of things in openssl/sha.h:
pg_shaXX_init(pg_shaXX_ctx *ctx, data);
pg_shaXX_update(pg_shaXX_ctx *ctx, uint8 *data, size_t len);
pg_shaXX_final(uint8 *dest, pg_shaXX_ctx *ctx);
Then think about shaXX as 1, 224, 256, 384 and 512.

Hence all those functions, moved to src/common, finish with the
following shape, take an init() one:
#ifdef USE_SSL
#define 
#endif
void
pg_shaXX_init(pg_shaXX_ctx *ctx)
{
#ifdef USE_SSL
SHAXX_Init((SHAXX_CTX *) ctx);
#else
//Here does the OpenBSD stuff, now part of pgcrypto
#endif
}

And that's really ugly, all the OpenBSD things that are used by
pgcrypto when the code is not built with --with-openssl gather into a
single place with parts wrapped around USE_SSL. A less ugly solution
would be to split that into two files, and one or the other gets
included in OBJS depending on if the build is done with or without
OpenSSL. We do a rather similar thing with fe/be-secure-openssl.c.

Another possibility is that we could say that SCRAM is designed to
work with TLS, as mentioned a bit upthread via the RFC, so we would
not support it in builds compiled without OpenSSL. I think that would
be a shame, but it would simplify all this refactoring juggling.

So, 3 possibilities here:
1) Use a single file src/common/sha.c that includes a set of functions
using USE_SSL
2) Have two files in src/common, one when build is used with OpenSSL,
and the second one when built-in methods are used
3) Disable the use of SCRAM when OpenSSL is not present in the build.

Opinions? My heart goes for 2) because 1) is ugly, and 3) is not
appealing in terms of flexibility.
-- 
Michael


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