Re: Different results between PostgreSQL and Oracle for "for update" statement

[Andy Fan]

Thank all of you for your great insight!

On Sat, Nov 21, 2020 at 9:04 AM Peter Geoghegan  wrote:

> On Fri, Nov 20, 2020 at 3:04 PM Andreas Karlsson 
> wrote:
> > I am sadly not familiar enough with Oracle or have access to any Oracle
> > license so I cannot comment on how Oracle have implemented their behvior
> > or what tradeoffs they have made.
>
> I bet that Oracle does a statement-level rollback for READ COMMITTED
> mode's conflict handling.


I'd agree with you about this point,  this difference can cause more
different
behavior between Postgres & Oracle (not just select .. for update).

create table dml(a int, b int);
insert into dml values(1, 1), (2,2);

-- session 1:
begin;
delete from dml where a in (select min(a) from dml);

--session 2:
delete from dml where a in (select min(a) from dml);

-- session 1:
commit;

In Oracle:  1 row deleted in sess 2.
In PG: 0 rows are deleted.


> I'm not sure if this means that it locks multiple rows or not.


This is something not really exists and you can ignore this part:)

About the statement level rollback,  Another difference is related.

create table t (a int primary key, b int);
begin;
insert into t values(1,1);
insert into t values(1, 1);
commit;

Oracle : t has 1 row, PG:  t has 0 row (since the whole transaction is
aborted).

I don't mean we need to be the same as Oracle, but to support a
customer who comes from Oracle, it would be good to know the
difference.

-- 
Best Regards
Andy Fan

Re: Implement for window functions

[Krasiyan Andreev]

Fixed patch attached, after new introduced conflicts.
Vik, can you add it to the next commitfest, to be able to test it.
Also, all tests from Oliver Ford's old patch also passed successfully.

На пт, 13.11.2020 г. в 0:44 ч. Vik Fearing  написа:

> On 11/12/20 11:35 PM, Krasiyan Andreev wrote:
> > Hi, it looks like Vik Fearing's patch does not apply anymore, because
> there
> > are many conflicts with recent changes, fixed patch attached.
>
> Thanks!  I've been away from this list for a while and I have some
> catching up to do.
> --
> Vik Fearing
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7d06b979eb..e69977de76 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19722,6 +19722,14 @@ SELECT count(*) FROM sometable;
about frame specifications.
   
 
+  
+   The functions lead, lag,
+   first_value, last_value, and
+   nth_value accept a null treatment option which is
+   RESPECT NULLS or IGNORE NULLS.
+   If this option is not specified, the default is RESPECT NULLS.
+  
+
   
When an aggregate function is used as a window function, it aggregates
over the rows within the current row's window frame.
@@ -19735,14 +19743,9 @@ SELECT count(*) FROM sometable;
 
   

-The SQL standard defines a RESPECT NULLS or
-IGNORE NULLS option for lead, lag,
-first_value, last_value, and
-nth_value.  This is not implemented in
-PostgreSQL: the behavior is always the
-same as the standard's default, namely RESPECT NULLS.
-Likewise, the standard's FROM FIRST or FROM LAST
-option for nth_value is not implemented: only the
+The SQL standard defines a FROM FIRST or FROM LAST
+option for nth_value.  This is not implemented in
+PostgreSQL: only the
 default FROM FIRST behavior is supported.  (You can achieve
 the result of FROM LAST by reversing the ORDER BY
 ordering.)
diff --git a/doc/src/sgml/ref/create_function.sgml b/doc/src/sgml/ref/create_function.sgml
index 3c1eaea651..31e08c26b4 100644
--- a/doc/src/sgml/ref/create_function.sgml
+++ b/doc/src/sgml/ref/create_function.sgml
@@ -28,6 +28,7 @@ CREATE [ OR REPLACE ] FUNCTION
   { LANGUAGE lang_name
 | TRANSFORM { FOR TYPE type_name } [, ... ]
 | WINDOW
+| TREAT NULLS
 | IMMUTABLE | STABLE | VOLATILE | [ NOT ] LEAKPROOF
 | CALLED ON NULL INPUT | RETURNS NULL ON NULL INPUT | STRICT
 | [ EXTERNAL ] SECURITY INVOKER | [ EXTERNAL ] SECURITY DEFINER
@@ -293,6 +294,17 @@ CREATE [ OR REPLACE ] FUNCTION
  
 
 
+
+ TREAT NULLS
+
+ 
+  TREAT NULLS indicates that the function is able
+  to handle the RESPECT NULLS and IGNORE
+  NULLS options.  Only window functions may specify this.
+  
+ 
+
+
 
  IMMUTABLE
  STABLE
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 3fdd87823e..685454c1ec 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -1770,6 +1770,8 @@ FROM generate_series(1,10) AS s(i);
 The syntax of a window function call is one of the following:
 
 
+function_name (expression , expression ... ) [ RESPECT NULLS | IGNORE NULLS ] OVER window_name
+function_name (expression , expression ... ) [ RESPECT NULLS | IGNORE NULLS ] OVER ( window_definition )
 function_name (expression , expression ... ) [ FILTER ( WHERE filter_clause ) ] OVER window_name
 function_name (expression , expression ... ) [ FILTER ( WHERE filter_clause ) ] OVER ( window_definition )
 function_name ( * ) [ FILTER ( WHERE filter_clause ) ] OVER window_name
@@ -1783,6 +1785,18 @@ FROM generate_series(1,10) AS s(i);
 [ ORDER BY expression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
 [ frame_clause ]
 
+   
+
+   
+
+ The versions with RESPECT NULLS or IGNORE
+ NULLS only apply to true window functions, whereas the versions
+ with FILTER only apply to aggregate functions used as
+ window functions.
+
+   
+
+   
 The optional frame_clause
 can be one of
 
diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index 7664bb6285..fea8778ec8 100644
--- a/src/backend/catalog/pg_aggregate.c
+++ b/src/backend/catalog/pg_aggregate.c
@@ -627,6 +627,7 @@ AggregateCreate(const char *aggName,
 	 * definable for agg) */
 			 false, /* isLeakProof */
 			 false, /* isStrict (not needed for agg) */
+			 false, /* null_treatment (not needed for agg) */
 			 PROVOLATILE_IMMUTABLE, /* volatility (not needed
 	 * for agg) */
 			 proparallel,
@@ -848,7 +849,7 @@ lookup_agg_function(List *fnName,
 			   nargs, input_types, false, false,
 			   &fnOid, rettype, &retset,
 			   &nvargs, &vatype,
-			   &true_oid_array, NULL);
+			   &true_oid_array, NULL, NULL);
 
 	/* only valid case is a normal function not returning a set */
 	if (fdresult != FUNCDETAIL_NORMAL || !OidIsValid(fnOid))
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
in

Re: Skip ExecCheckRTPerms in CTAS with no data

[Michael Paquier]

On Fri, Nov 20, 2020 at 03:04:57PM +0530, Bharath Rupireddy wrote:
> Thanks! Attaching the patch. Please review it.

Thanks.  I have removed the references to the INSERT check in the
comments and the docs, because that would be confusing as it refers
to something we don't do anymore now with this patch, reordered the
tests and applied the patch.
--
Michael


signature.asc
Description: PGP signature

Connection using ODBC and SSL

[Corbit, Dann]

I figured out that my TLS version was too low in the libpq call and increased 
it to TLS v1.1
Should I go to 1.2?  I am wondering because I do not want to limit 
compatibility.

Once I got past that hurdle, I am getting the error "ssl error: the certificate 
verify failed"
Since I built the certificates myself self-signed, I am assuming I did 
something that Postgres does not like.
I should mention that I am using the Windows environment for testing (I will 
test Linux after Windows succeeds).

I would like to have all my certificates and keys on the same machine 
(localhost for local connections and dcorbit for tcp/ip).
I found a couple tutorials and tried them but it failed.
I saw one document that said the common name should be the postgres user name 
and that it should also be the connecting machine name.  Is that correct?
Is there a document or tutorial that explains the correct steps?
Equally important, is there a way to get more complete diagnostics when 
something goes wrong (like WHY did the certificate verify fail)?

Re: Different results between PostgreSQL and Oracle for "for update" statement

[Pavel Stehule]

so 21. 11. 2020 v 9:59 odesílatel Andy Fan 
napsal:

> Thank all of you for your great insight!
>
> On Sat, Nov 21, 2020 at 9:04 AM Peter Geoghegan  wrote:
>
>> On Fri, Nov 20, 2020 at 3:04 PM Andreas Karlsson 
>> wrote:
>> > I am sadly not familiar enough with Oracle or have access to any Oracle
>> > license so I cannot comment on how Oracle have implemented their behvior
>> > or what tradeoffs they have made.
>>
>> I bet that Oracle does a statement-level rollback for READ COMMITTED
>> mode's conflict handling.
>
>
> I'd agree with you about this point,  this difference can cause more
> different
> behavior between Postgres & Oracle (not just select .. for update).
>
> create table dml(a int, b int);
> insert into dml values(1, 1), (2,2);
>
> -- session 1:
> begin;
> delete from dml where a in (select min(a) from dml);
>
> --session 2:
> delete from dml where a in (select min(a) from dml);
>
> -- session 1:
> commit;
>
> In Oracle:  1 row deleted in sess 2.
> In PG: 0 rows are deleted.
>
>
>> I'm not sure if this means that it locks multiple rows or not.
>
>
> This is something not really exists and you can ignore this part:)
>
> About the statement level rollback,  Another difference is related.
>
> create table t (a int primary key, b int);
> begin;
> insert into t values(1,1);
> insert into t values(1, 1);
> commit;
>
> Oracle : t has 1 row, PG:  t has 0 row (since the whole transaction is
> aborted).
>
> I don't mean we need to be the same as Oracle, but to support a
> customer who comes from Oracle, it would be good to know the
> difference.
>

yes, it would be nice to be better documented, somewhere - it should not be
part of Postgres documentation. Unfortunately, people who know Postgres
perfectly do not have the same knowledge about Oracle.

Some differences are documented in Orafce documentation
https://github.com/orafce/orafce/tree/master/doc

but I am afraid so there is nothing about the different behaviour of
snapshots.

Regards

Pavel


> --
> Best Regards
> Andy Fan
>

Re: jit and explain nontext

[Justin Pryzby]

On Sat, Nov 21, 2020 at 08:39:11AM +0100, Peter Eisentraut wrote:
> On 2020-11-20 17:16, Justin Pryzby wrote:
> > It matters if it was planned with jit but executed without jit.
> > 
> > postgres=# DEALLOCATE p; SET jit=on; SET jit_above_cost=0; prepare p as 
> > select from generate_series(1,9); explain(format yaml) execute p; SET 
> > jit=off; explain(format yaml) execute p;
> > 
> > Patched shows this for both explains:
> > JIT:  +
> >   Functions: 3+
> > 
> > Unpatched shows only in the first case.
> 
> In this context, I don't see the point of this change.  If you set jit=off
> explicitly, then there is no need to clutter the EXPLAIN output with a bunch
> of zeroes about JIT.

I think the idea is that someone should be able to parse the YAML/XML/other
output by writing something like a['JIT']['Functions'] rather than something
like a.get('JIT',{}).get('Functions',0)

The standard seems to be that parameters that can change during execution
should change the *values* in the non-text output, but the *keys* should not
disappear just because (for example) parallel workers weren't available, or
someone (else) turned off jit.  We had discussion about this earlier in the
year:
https://www.postgresql.org/message-id/20200728033622.gc20...@telsasoft.com

(Since it's machine-readable output, Key: 0 is Consistency and Completeness,
not Clutter.)

-- 
Justin

Re: Connection using ODBC and SSL

[Tom Lane]

"Corbit, Dann"  writes:
> I figured out that my TLS version was too low in the libpq call and increased 
> it to TLS v1.1
> Should I go to 1.2?  I am wondering because I do not want to limit 
> compatibility.

PG 13 and up consider that 1.2 is the *minimum* secure version.
Quoting from the commit log:

Change libpq's default ssl_min_protocol_version to TLSv1.2.

When we initially created this parameter, in commit ff8ca5fad, we left
the default as "allow any protocol version" on grounds of backwards
compatibility.  However, that's inconsistent with the backend's default
since b1abfec82; protocol versions prior to 1.2 are not considered very
secure; and OpenSSL has had TLSv1.2 support since 2012, so the number
of PG servers that need a lesser minimum is probably quite small.

On top of those things, it emerges that some popular distros (including
Debian and RHEL) set MinProtocol=TLSv1.2 in openssl.cnf.  Thus, far
from having "allow any protocol version" behavior in practice, what
we actually have as things stand is a platform-dependent lower limit.

So, change our minds and set the min version to TLSv1.2.  Anybody
wanting to connect with a new libpq to a pre-2012 server can either
set ssl_min_protocol_version=TLSv1 or accept the fallback to non-SSL.

Back-patch to v13 where the aforementioned patches appeared.

> Once I got past that hurdle, I am getting the error "ssl error: the 
> certificate verify failed"
> Since I built the certificates myself self-signed, I am assuming I did 
> something that Postgres does not like.

The process in our docs worked for me last time I tried it:

https://www.postgresql.org/docs/current/ssl-tcp.html#SSL-CERTIFICATE-CREATION

regards, tom lane

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Tom Lane]

Michael Paquier  writes:
> Indeed, this could go.  There is a recursive call for views, but in
> order to maintain compatibility with that we can just remove one
> function and move the second to use a regclass as argument, like the
> attached, while removing setLastTid().  Any thoughts?

Considering that we're preserving this only for backwards compatibility,
I doubt that changing the signature is a good idea.  It maybe risks
breaking something, and the ODBC driver is hardly going to notice
any improved ease-of-use.

regards, tom lane

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Andres Freund]

Hi,

+1 for getting rid of whatever we can without too much trouble.

On 2020-11-21 13:13:35 -0500, Tom Lane wrote:
> Michael Paquier  writes:
> > Indeed, this could go.  There is a recursive call for views, but in
> > order to maintain compatibility with that we can just remove one
> > function and move the second to use a regclass as argument, like the
> > attached, while removing setLastTid().  Any thoughts?
> 
> Considering that we're preserving this only for backwards compatibility,
> I doubt that changing the signature is a good idea.  It maybe risks
> breaking something, and the ODBC driver is hardly going to notice
> any improved ease-of-use.

+1.

Regards,

Andres

Re: Connection using ODBC and SSL

[Andrew Dunstan]

On 11/20/20 4:54 PM, Corbit, Dann wrote:
>
> I would like to have all my certificates and keys on the same machine
> (localhost for local connections and dcorbit for tcp/ip).
> I found a couple tutorials and tried them but it failed.
> I saw one document that said the common name should be the postgres
> user name and that it should also be the connecting machine name.  Is
> that correct?
> Is there a document or tutorial that explains the correct steps?



I did a webinar about a year ago that went into some detail about what
you need in the CN, where the certificates go, etc.


See

(Yes, this is a corporate webinar, sorry about that)




> Equally important, is there a way to get more complete diagnostics
> when something goes wrong (like WHY did the certificate verify fail)?
>

The diagnostics in the Postgres log are usually fairly explanatory.



cheers


andrew

bug in pageinspect's "tuple data" feature

[Alvaro Herrera]

If you have a sufficiently broken data page, pageinspect throws an
error when trying to examine the page:

ERROR:  invalid memory alloc request size 18446744073709551451

This is pretty unhelpful; it would be better not to try to print the
data instead of dying.  With that, at least you can know where the
problem is.

This was introduced in d6061f83a166 (2015).  Proposed patch to fix it
(by having the code print a null "data" instead of dying) is attached.

-- 
Álvaro Herrera
diff --git a/contrib/pageinspect/heapfuncs.c b/contrib/pageinspect/heapfuncs.c
index 64a6e351d5..909fdee406 100644
--- a/contrib/pageinspect/heapfuncs.c
+++ b/contrib/pageinspect/heapfuncs.c
@@ -228,11 +228,17 @@ heap_page_items(PG_FUNCTION_ARGS)
 
 			/* Copy raw tuple data into bytea attribute */
 			tuple_data_len = lp_len - tuphdr->t_hoff;
-			tuple_data_bytea = (bytea *) palloc(tuple_data_len + VARHDRSZ);
-			SET_VARSIZE(tuple_data_bytea, tuple_data_len + VARHDRSZ);
-			memcpy(VARDATA(tuple_data_bytea), (char *) tuphdr + tuphdr->t_hoff,
-   tuple_data_len);
-			values[13] = PointerGetDatum(tuple_data_bytea);
+			if (tuple_data_len < BLCKSZ)
+			{
+tuple_data_bytea = (bytea *) palloc(tuple_data_len + VARHDRSZ);
+SET_VARSIZE(tuple_data_bytea, tuple_data_len + VARHDRSZ);
+memcpy(VARDATA(tuple_data_bytea), (char *) tuphdr + tuphdr->t_hoff,
+	   tuple_data_len);
+values[13] = PointerGetDatum(tuple_data_bytea);
+nulls[13] = false;
+			}
+			else
+nulls[13] = true;
 
 			/*
 			 * We already checked that the item is completely within the raw

Re: [PATCH] remove pg_standby

[Justin Pryzby]

On Fri, Nov 20, 2020 at 05:26:54PM +0100, Peter Eisentraut wrote:
> On 2020-10-29 03:44, Justin Pryzby wrote:
> > diff --git a/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml
> > index 4e833d79ef..be4292ec33 100644
> > --- a/doc/src/sgml/contrib.sgml
> > +++ b/doc/src/sgml/contrib.sgml
> > @@ -199,6 +199,5 @@ pages.
> >  part of the core PostgreSQL distribution.
> > 
> > - &pgstandby;
> >
> >   
> 
> With this removal, that section becomes empty.  So you probably want to
> clean up or reorganize this a bit.
> 
> See https://www.postgresql.org/docs/devel/contrib-prog.html for the context.

Oops.  I guess I'd write something like this.  If we just remove it, then
there'd no place to add a new server application, and "client applications"
would be the only subsection.

-- 
Justin
>From 2cc4d8adc0d8e2f7bd4c21989a85e6e97b1d2fd6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Fri, 20 Nov 2020 14:31:15 -0600
Subject: [PATCH v2 1/2] Add missing word: 'are'

Should backpatch
---
 doc/src/sgml/contrib.sgml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml
index 4e833d79ef..ae2759be55 100644
--- a/doc/src/sgml/contrib.sgml
+++ b/doc/src/sgml/contrib.sgml
@@ -180,7 +180,7 @@ pages.
applications in contrib.  They can be run from anywhere,
independent of where the database server resides.  See
also  for information about client
-   applications that part of the core PostgreSQL
+   applications that are part of the core PostgreSQL
distribution.
   
 
@@ -196,7 +196,7 @@ pages.
applications in contrib.  They are typically run on the
host where the database server resides.  See also  for information about server applications that
-   part of the core PostgreSQL distribution.
+   are part of the core PostgreSQL distribution.
   
 
  &pgstandby;
-- 
2.17.0

>From 626fba9074d6bd0e0d6615c0be5932ed63603455 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Mon, 26 Oct 2020 16:37:46 -0500
Subject: [PATCH v2 2/2] Retire pg_standby..

..since pg9.0, use builtin streaming replication protocol with warm/hot standby
---
 contrib/Makefile|   1 -
 contrib/pg_standby/.gitignore   |   1 -
 contrib/pg_standby/Makefile |  20 -
 contrib/pg_standby/pg_standby.c | 907 
 doc/src/sgml/contrib.sgml   |   7 +-
 doc/src/sgml/filelist.sgml  |   1 -
 doc/src/sgml/high-availability.sgml |  14 +-
 doc/src/sgml/pgstandby.sgml | 394 
 src/tools/msvc/Mkvcbuild.pm |   4 +-
 9 files changed, 7 insertions(+), 1342 deletions(-)
 delete mode 100644 contrib/pg_standby/.gitignore
 delete mode 100644 contrib/pg_standby/Makefile
 delete mode 100644 contrib/pg_standby/pg_standby.c
 delete mode 100644 doc/src/sgml/pgstandby.sgml

diff --git a/contrib/Makefile b/contrib/Makefile
index 7a4866e338..cdc041c7db 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -33,7 +33,6 @@ SUBDIRS = \
 		pg_buffercache	\
 		pg_freespacemap \
 		pg_prewarm	\
-		pg_standby	\
 		pg_stat_statements \
 		pg_surgery	\
 		pg_trgm		\
diff --git a/contrib/pg_standby/.gitignore b/contrib/pg_standby/.gitignore
deleted file mode 100644
index a401b085a8..00
--- a/contrib/pg_standby/.gitignore
+++ /dev/null
@@ -1 +0,0 @@
-/pg_standby
diff --git a/contrib/pg_standby/Makefile b/contrib/pg_standby/Makefile
deleted file mode 100644
index 87732bedf1..00
--- a/contrib/pg_standby/Makefile
+++ /dev/null
@@ -1,20 +0,0 @@
-# contrib/pg_standby/Makefile
-
-PGFILEDESC = "pg_standby - supports creation of a warm standby"
-PGAPPICON = win32
-
-PROGRAM = pg_standby
-OBJS = \
-	$(WIN32RES) \
-	pg_standby.o
-
-ifdef USE_PGXS
-PG_CONFIG = pg_config
-PGXS := $(shell $(PG_CONFIG) --pgxs)
-include $(PGXS)
-else
-subdir = contrib/pg_standby
-top_builddir = ../..
-include $(top_builddir)/src/Makefile.global
-include $(top_srcdir)/contrib/contrib-global.mk
-endif
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
deleted file mode 100644
index c9f33e4254..00
--- a/contrib/pg_standby/pg_standby.c
+++ /dev/null
@@ -1,907 +0,0 @@
-/*
- * contrib/pg_standby/pg_standby.c
- *
- *
- * pg_standby.c
- *
- * Production-ready example of how to create a Warm Standby
- * database server using continuous archiving as a
- * replication mechanism
- *
- * We separate the parameters for archive and nextWALfile
- * so that we can check the archive exists, even if the
- * WAL file doesn't (yet).
- *
- * This program will be executed once in full for each file
- * requested by the warm standby server.
- *
- * It is designed to cater to a variety of needs, as well
- * providing a customizable section.
- *
- * Original author:		Simon Riggs  si...@2ndquadrant.com
- * Current maintainer:	Simon Riggs
- */
-#include "postgres_fe.h"
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#include "access/xlog_internal.h"
-#include "pg_getopt.h"
-
-con

Re: Different results between PostgreSQL and Oracle for "for update" statement

[Peter Geoghegan]

On Sat, Nov 21, 2020 at 12:58 AM Andy Fan  wrote:
> I don't mean we need to be the same as Oracle, but to support a
> customer who comes from Oracle, it would be good to know the
> difference.

Actually, it is documented here:
https://www.postgresql.org/docs/devel/transaction-iso.html

The description starts with: "UPDATE, DELETE, SELECT FOR UPDATE, and
SELECT FOR SHARE commands behave the same as SELECT in terms of
searching for target rows...".

I imagine that the number of application developers that are aware of
this specific aspect of transaction isolation in PostgreSQL (READ
COMMITTED conflict handling/EvalPlanQual()) is extremely small. In
practice it doesn't come up that often. Though Postgres hackers tend
to think about it a lot because it is hard to maintain.

I'm not saying that that's good or bad. Just that that has been my experience.

I am sure that some application developers really do understand the
single most important thing about READ COMMITTED mode's behavior: each
new command gets its own MVCC snapshot. But I believe that Oracle is
no different. So in practice application developers probably don't
notice any difference between READ COMMITTED mode in practically all
cases. (Again, just my opinion.)

-- 
Peter Geoghegan

Re: Strange behavior with polygon and NaN

[Tom Lane]

I went ahead and pushed 0001 and 0003 (the latter in two parts), since
they didn't seem particularly controversial to me.  Just to keep the
cfbot from whining, here's a rebased version of 0002.

regards, tom lane

diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index c1dc511a1a..d9f577bd8b 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -2719,9 +2719,14 @@ lseg_interpt_line(Point *result, LSEG *lseg, LINE *line)
  *---*/
 
 /*
- * If *result is not NULL, it is set to the intersection point of a
- * perpendicular of the line through the point.  Returns the distance
- * of those two points.
+ * Determine the closest point on the given line to the given point.
+ * If result is not NULL, *result is set to the coordinates of that point.
+ * In any case, returns the distance from the point to the line.
+ * Returns NaN for any ill-defined value.
+ *
+ * NOTE: in some cases the distance is well defined (i.e., infinity)
+ * even though the specific closest point is not.  Hence, if you care
+ * about whether the point is well-defined, check its coordinates for NaNs.
  */
 static float8
 line_closept_point(Point *result, LINE *line, Point *point)
@@ -2729,17 +2734,30 @@ line_closept_point(Point *result, LINE *line, Point *point)
 	Point		closept;
 	LINE		tmp;
 
+	/*
+	 * If it is unclear whether the point is on the line or not, then the
+	 * results are ill-defined.  This eliminates cases where any of the given
+	 * coordinates are NaN, as well as cases where infinite coordinates give
+	 * rise to Inf - Inf, 0 * Inf, etc.
+	 */
+	if (unlikely(isnan(float8_pl(float8_pl(float8_mul(line->A, point->x),
+		   float8_mul(line->B, point->y)),
+ line->C
+	{
+		if (result != NULL)
+			result->x = result->y = get_float8_nan();
+		return get_float8_nan();
+	}
+
 	/*
 	 * We drop a perpendicular to find the intersection point.  Ordinarily we
-	 * should always find it, but that can fail in the presence of NaN
-	 * coordinates, and perhaps even from simple roundoff issues.
+	 * should always find it, but perhaps roundoff issues could prevent that.
 	 */
 	line_construct(&tmp, point, line_invsl(line));
 	if (!line_interpt_line(&closept, &tmp, line))
 	{
 		if (result != NULL)
-			*result = *point;
-
+			result->x = result->y = get_float8_nan();
 		return get_float8_nan();
 	}
 
@@ -2758,7 +2776,9 @@ close_pl(PG_FUNCTION_ARGS)
 
 	result = (Point *) palloc(sizeof(Point));
 
-	if (isnan(line_closept_point(result, line, pt)))
+	(void) line_closept_point(result, line, pt);
+
+	if (unlikely(isnan(result->x) || isnan(result->y)))
 		PG_RETURN_NULL();
 
 	PG_RETURN_POINT_P(result);
diff --git a/src/test/regress/expected/geometry.out b/src/test/regress/expected/geometry.out
index c4f853ae9f..6210075bc1 100644
--- a/src/test/regress/expected/geometry.out
+++ b/src/test/regress/expected/geometry.out
@@ -1070,9 +1070,9 @@ SELECT p.f1, l.s, p.f1 ## l.s FROM POINT_TBL p, LINE_TBL l;
  (1e+300,Infinity) | {0,-1,5}  | (1e+300,5)
  (1e+300,Infinity) | {1,0,5}   | 
  (1e+300,Infinity) | {0,3,0}   | (1e+300,0)
- (1e+300,Infinity) | {1,-1,0}  | (Infinity,NaN)
- (1e+300,Infinity) | {-0.4,-1,-6}  | (-Infinity,NaN)
- (1e+300,Infinity) | {-0.000184615384615,-1,15.3846153846} | (-Infinity,NaN)
+ (1e+300,Infinity) | {1,-1,0}  | 
+ (1e+300,Infinity) | {-0.4,-1,-6}  | 
+ (1e+300,Infinity) | {-0.000184615384615,-1,15.3846153846} | 
  (1e+300,Infinity) | {3,NaN,5} | 
  (1e+300,Infinity) | {NaN,NaN,NaN} | 
  (1e+300,Infinity) | {0,-1,3}  | (1e+300,3)

Re: [PATCH] Covering SPGiST index

[Tom Lane]

Pavel Borisov  writes:
> The way that seems acceptable to me is to add (optional) nulls mask into
> the end of existing style SpGistLeafTuple header and use indextuple
> routines to attach attributes after it. In this case, we can reduce the
> amount of code at the cost of adding one extra MAXALIGN size to the overall
> tuple size on 32-bit arch as now tuple header size of 12 bit already fits 3
> MAXALIGNS (on 64 bit the header now is shorter than 2 maxaligns (12 bytes
> of 16) and nulls mask will be free of cost). If you mean this I try to make
> changes soon. What do you think of it?

Yeah, that was pretty much the same conclusion I came to.  For
INDEX_MAX_KEYS values up to 32, the nulls bitmap will fit into what's
now padding space on 64-bit machines.  For backwards compatibility,
we'd have to be careful that the code knows there's no nulls bitmap in
an index without included columns, so I'm not sure how messy that will
be.  But it's worth trying that way to see how it comes out.

regards, tom lane

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Michael Paquier]

On Sat, Nov 21, 2020 at 01:13:35PM -0500, Tom Lane wrote:
> Considering that we're preserving this only for backwards compatibility,
> I doubt that changing the signature is a good idea.  It maybe risks
> breaking something, and the ODBC driver is hardly going to notice
> any improved ease-of-use.

So, what you are basically saying is to switch currtid_byreloid() to
become a function local to tid.c.  And then have just
currtid_byrelname() and currtid_for_view() call that, right?
--
Michael


signature.asc
Description: PGP signature

Re: Different results between PostgreSQL and Oracle for "for update" statement

[Andy Fan]

On Sat, Nov 21, 2020 at 11:27 PM Pavel Stehule 
wrote:

>
>
> so 21. 11. 2020 v 9:59 odesílatel Andy Fan 
> napsal:
>
>> Thank all of you for your great insight!
>>
>> On Sat, Nov 21, 2020 at 9:04 AM Peter Geoghegan  wrote:
>>
>>> On Fri, Nov 20, 2020 at 3:04 PM Andreas Karlsson 
>>> wrote:
>>> > I am sadly not familiar enough with Oracle or have access to any Oracle
>>> > license so I cannot comment on how Oracle have implemented their
>>> behvior
>>> > or what tradeoffs they have made.
>>>
>>> I bet that Oracle does a statement-level rollback for READ COMMITTED
>>> mode's conflict handling.
>>
>>
>> I'd agree with you about this point,  this difference can cause more
>> different
>> behavior between Postgres & Oracle (not just select .. for update).
>>
>> create table dml(a int, b int);
>> insert into dml values(1, 1), (2,2);
>>
>> -- session 1:
>> begin;
>> delete from dml where a in (select min(a) from dml);
>>
>> --session 2:
>> delete from dml where a in (select min(a) from dml);
>>
>> -- session 1:
>> commit;
>>
>> In Oracle:  1 row deleted in sess 2.
>> In PG: 0 rows are deleted.
>>
>>
>>> I'm not sure if this means that it locks multiple rows or not.
>>
>>
>> This is something not really exists and you can ignore this part:)
>>
>> About the statement level rollback,  Another difference is related.
>>
>> create table t (a int primary key, b int);
>> begin;
>> insert into t values(1,1);
>> insert into t values(1, 1);
>> commit;
>>
>> Oracle : t has 1 row, PG:  t has 0 row (since the whole transaction is
>> aborted).
>>
>> I don't mean we need to be the same as Oracle, but to support a
>> customer who comes from Oracle, it would be good to know the
>> difference.
>>
>
> yes, it would be nice to be better documented, somewhere - it should not
> be part of Postgres documentation. Unfortunately, people who know Postgres
> perfectly do not have the same knowledge about Oracle.
>
> Some differences are documented in Orafce documentation
> https://github.com/orafce/orafce/tree/master/doc
>
>
orafce project is awesome!


> but I am afraid so there is nothing about the different behaviour of
> snapshots.
>
>
https://github.com/orafce/orafce/pull/120 is opened for this.

-- 
Best Regards
Andy Fan

Re: Different results between PostgreSQL and Oracle for "for update" statement

[Andy Fan]

On Sun, Nov 22, 2020 at 5:56 AM Peter Geoghegan  wrote:

> On Sat, Nov 21, 2020 at 12:58 AM Andy Fan 
> wrote:
> > I don't mean we need to be the same as Oracle, but to support a
> > customer who comes from Oracle, it would be good to know the
> > difference.
>
> Actually, it is documented here:
> https://www.postgresql.org/docs/devel/transaction-iso.html
>
> The description starts with: "UPDATE, DELETE, SELECT FOR UPDATE, and
> SELECT FOR SHARE commands behave the same as SELECT in terms of
> searching for target rows...".
>
> I imagine that the number of application developers that are aware of
> this specific aspect of transaction isolation in PostgreSQL (READ
> COMMITTED conflict handling/EvalPlanQual()) is extremely small. In
> practice it doesn't come up that often.


Totally agree with that.


> Though Postgres hackers tend to think about it a lot because it is hard to
> maintain.
>

Hackers may care about this if they run into a real user case :)


> I'm not saying that that's good or bad. Just that that has been my
> experience.
>
> I am sure that some application developers really do understand the
> single most important thing about READ COMMITTED mode's behavior: each
> new command gets its own MVCC snapshot. But I believe that Oracle is
> no different. So in practice application developers probably don't
> notice any difference between READ COMMITTED mode in practically all
> cases. (Again, just my opinion.)
>
> --
> Peter Geoghegan
>


-- 
Best Regards
Andy Fan

Re: Removal of currtid()/currtid2() and some table AM cleanup

[Tom Lane]

Michael Paquier  writes:
> So, what you are basically saying is to switch currtid_byreloid() to
> become a function local to tid.c.  And then have just
> currtid_byrelname() and currtid_for_view() call that, right?

Yeah, that sounds about right.

regards, tom lane

Printing backtrace of postgres processes

[vignesh C]

Hi,

I would like to propose getting the callstack of the postgres process
by connecting to the server. This helps us in diagnosing the problems
from a customer environment in case of hung process or in case of long
running process.
The idea here is to implement & expose pg_print_callstack function,
internally what this function does is, the connected backend will send
SIGUSR1 signal by setting PMSIGNAL_BACKTRACE_EMIT to the postmaster
process. Postmaster process will send a SIGUSR1 signal to the process
by setting PROCSIG_BACKTRACE_PRINT if the process has access to
ProcSignal. As syslogger process & Stats process don't have access to
ProcSignal, multiplexing with SIGUSR1 is not possible for these
processes, hence SIGUSR2 signal will be sent for these processes. Once
the process receives this signal it will log the backtrace of the
process.
Attached is a WIP patch for the same.
Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
From c1006110bdeac2135d1c8e9220f65d50cd49ab63 Mon Sep 17 00:00:00 2001
From: Vignesh C 
Date: Sun, 22 Nov 2020 05:58:24 +0530
Subject: [PATCH] Print backtrace of postgres process that are part of this
 instance.

The idea here is to implement & expose pg_print_callstack function, internally
what this function does is, the connected backend will send SIGUSR1 signal by
setting PMSIGNAL_BACKTRACE_EMIT to the postmaster process. Postmaster process
will send SIGUSR1 signal to process by setting PROCSIG_BACKTRACE_PRINT if the
process that have access to ProcSignal. As syslogger process & Stats process
don't have access to ProcSignal, multiplexing with SIGUSR1 is not possible
for these processes, hence SIGUSR2 signal will be sent for these process.
Once the process receives this signal it will log the backtrace of the process.
---
 src/backend/postmaster/pgstat.c   | 19 ++-
 src/backend/postmaster/postmaster.c   | 20 
 src/backend/postmaster/syslogger.c| 16 +++-
 src/backend/storage/ipc/procsignal.c  | 28 
 src/backend/storage/ipc/signalfuncs.c | 22 ++
 src/backend/tcop/postgres.c   | 31 +++
 src/include/catalog/pg_proc.dat   |  6 +-
 src/include/storage/pmsignal.h|  2 ++
 src/include/storage/procsignal.h  |  2 ++
 src/include/tcop/tcopprot.h   |  2 ++
 10 files changed, 145 insertions(+), 3 deletions(-)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index e76e627..bd38264 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -62,6 +62,7 @@
 #include "storage/pg_shmem.h"
 #include "storage/procsignal.h"
 #include "storage/sinvaladt.h"
+#include "tcop/tcopprot.h"
 #include "utils/ascii.h"
 #include "utils/guc.h"
 #include "utils/memutils.h"
@@ -372,6 +373,8 @@ static void pgstat_recv_checksum_failure(PgStat_MsgChecksumFailure *msg, int len
 static void pgstat_recv_replslot(PgStat_MsgReplSlot *msg, int len);
 static void pgstat_recv_tempfile(PgStat_MsgTempFile *msg, int len);
 
+static void sigUsr2Handler(SIGNAL_ARGS);
+
 /* 
  * Public functions called from postmaster follow
  * 
@@ -4666,7 +4669,7 @@ PgstatCollectorMain(int argc, char *argv[])
 	pqsignal(SIGALRM, SIG_IGN);
 	pqsignal(SIGPIPE, SIG_IGN);
 	pqsignal(SIGUSR1, SIG_IGN);
-	pqsignal(SIGUSR2, SIG_IGN);
+	pqsignal(SIGUSR2, sigUsr2Handler);
 	/* Reset some signals that are accepted by postmaster but not here */
 	pqsignal(SIGCHLD, SIG_DFL);
 	PG_SETMASK(&UnBlockSig);
@@ -7242,3 +7245,17 @@ pgstat_count_slru_truncate(int slru_idx)
 {
 	slru_entry(slru_idx)->m_truncate += 1;
 }
+
+/*
+ * sigUsr2Handler
+ *
+ * handle SIGUSR2 signal to print call stack of pgstat process.
+ */
+static void
+sigUsr2Handler(SIGNAL_ARGS)
+{
+	int			save_errno = errno;
+
+	LogBackTrace();
+	errno = save_errno;
+}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index b7799ed..dd7c930 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5149,6 +5149,26 @@ sigusr1_handler(SIGNAL_ARGS)
 		StartWorkerNeeded = true;
 	}
 
+	/* Process backtrace emit signal. */
+	if (CheckPostmasterSignal(PMSIGNAL_BACKTRACE_EMIT))
+	{
+		EmitProcSignalPrintCallStack();
+
+		/*
+		 * Pgstat process & syslogger process do not have access to ProcSignal,
+		 * multiplexing with SIGUSR1 is not possible for these processes so send
+		 * SIGUSR2 signal for them as multiplexing with SIGUSR1 is not possible.
+		 */
+		if (PgStatPID)
+			kill(PgStatPID, SIGUSR2);
+
+		if (SysLoggerPID)
+			kill(SysLoggerPID, SIGUSR2);
+
+		/* Print call stack for postmaster process. */
+		LogBackTrace();
+	}
+
 	/*
 	 * RECOVERY_STARTED and BEGIN_HOT_STANDBY signals are ignored in
 	 * unexpected states. If the startup process quickly starts up, completes
diff -

Re: Printing backtrace of postgres processes

[Tom Lane]

vignesh C  writes:
> The idea here is to implement & expose pg_print_callstack function,
> internally what this function does is, the connected backend will send
> SIGUSR1 signal by setting PMSIGNAL_BACKTRACE_EMIT to the postmaster
> process. Postmaster process will send a SIGUSR1 signal to the process
> by setting PROCSIG_BACKTRACE_PRINT if the process has access to
> ProcSignal. As syslogger process & Stats process don't have access to
> ProcSignal, multiplexing with SIGUSR1 is not possible for these
> processes, hence SIGUSR2 signal will be sent for these processes. Once
> the process receives this signal it will log the backtrace of the
> process.

Surely this is *utterly* unsafe.  You can't do that sort of stuff in
a signal handler.

It might be all right to set a flag that would cause the next
CHECK_FOR_INTERRUPTS to print a backtrace, but I'm not sure
how useful that really is.

The proposed postmaster.c addition seems quite useless, as there
is exactly one stack trace it could ever log.

I would like to see some discussion of the security implications
of such a feature, as well.  ("There aren't any" is the wrong
answer.)

regards, tom lane