Re: Fix help option of contrib/oid2name

[Tatsuro Yamada]

On 2018/08/17 12:42, Tatsuro Yamada wrote:

On 2018/08/17 11:47, Michael Paquier wrote:

On Thu, Aug 16, 2018 at 08:57:57PM +0900, Michael Paquier wrote:

I agree on both points.  Any objections if I apply what's proposed here
on HEAD?


I have been looking at this patch.  And while consistency is nice, I
think that if we are cleaning up this stuff we could do a bit more to
be more consistent with the other binary tools:
- Addition of long options for connection options.
- removal of -h, and use it for host value instead of "-H".
- Use "USERNAME" instead of "NAME" in help().

vacuumlo could also be improved a bit...


Yeah, I'll revise the patch based on your suggestions.


I created WIP patch for oid2name and vacuumlo includes followings:

  oid2name and vacuumlo
  - Add long options

  only oid2name
  - Replace -H with -h
  - Replace NAME with USERNAME

I'll revise their documents and create new patch next week, probably. :)

Regards,
Tatsuro Yamada

diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index 63e360c4c5..3501a9c2cc 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -60,8 +60,25 @@ void		sql_exec_dumpalltbspc(PGconn *, struct options *);
 void
 get_opts(int argc, char **argv, struct options *my_opts)
 {
+	static struct option long_options[] = {
+		{"host", required_argument, NULL, 'h'},
+		{"port", required_argument, NULL, 'p'},
+		{"username", required_argument, NULL, 'U'},
+		{"dbname", required_argument, NULL, 'd'},
+		{"tablename", required_argument, NULL, 't'},
+		{"oid", required_argument, NULL, 'o'},
+		{"filenode", no_argument, NULL, 'f'},
+		{"quiet", no_argument, NULL, 'q'},
+		{"system", no_argument, NULL, 'S'},
+		{"extend", no_argument, NULL, 'x'},
+		{"index", no_argument, NULL, 'i'},
+		{"tablespace", no_argument, NULL, 's'},
+		{NULL, 0, NULL, 0}
+	};
+
 	int			c;
 	const char *progname;
+	int			optindex;
 
 	progname = get_progname(argv[0]);
 
@@ -93,10 +110,25 @@ get_opts(int argc, char **argv, struct options *my_opts)
 	}
 
 	/* get opts */
-	while ((c = getopt(argc, argv, "H:p:U:d:t:o:f:qSxish")) != -1)
+	while ((c = getopt_long(argc, argv, "h:p:U:d:t:o:f:qSxis", long_options, &optindex)) != -1)
 	{
 		switch (c)
 		{
+/* host to connect to */
+			case 'h':
+my_opts->hostname = pg_strdup(optarg);
+break;
+
+/* port to connect to on remote host */
+			case 'p':
+my_opts->port = pg_strdup(optarg);
+break;
+
+/* username */
+			case 'U':
+my_opts->username = pg_strdup(optarg);
+break;
+
 /* specify the database */
 			case 'd':
 my_opts->dbname = pg_strdup(optarg);
@@ -122,46 +154,26 @@ get_opts(int argc, char **argv, struct options *my_opts)
 my_opts->quiet = true;
 break;
 
-/* host to connect to */
-			case 'H':
-my_opts->hostname = pg_strdup(optarg);
-break;
-
-/* port to connect to on remote host */
-			case 'p':
-my_opts->port = pg_strdup(optarg);
-break;
-
-/* username */
-			case 'U':
-my_opts->username = pg_strdup(optarg);
-break;
-
 /* display system tables */
 			case 'S':
 my_opts->systables = true;
 break;
 
-/* also display indexes */
-			case 'i':
-my_opts->indexes = true;
-break;
-
 /* display extra columns */
 			case 'x':
 my_opts->extended = true;
 break;
 
+/* also display indexes */
+			case 'i':
+my_opts->indexes = true;
+break;
+
 /* dump tablespaces only */
 			case 's':
 my_opts->tablespaces = true;
 break;
 
-			case 'h':
-help(progname);
-exit(0);
-break;
-
 			default:
 fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 exit(1);
@@ -176,20 +188,21 @@ help(const char *progname)
 		   "Usage:\n"
 		   "  %s [OPTION]...\n"
 		   "\nOptions:\n"
-		   "  -d DBNAME  database to connect to\n"
-		   "  -f FILENODEshow info for table with given file node\n"
-		   "  -H HOSTNAMEdatabase server host or socket directory\n"
-		   "  -i show indexes and sequences too\n"
-		   "  -o OID show info for table with given OID\n"
-		   "  -p PORTdatabase server port number\n"
-		   "  -q quiet (don't show headers)\n"
-		   "  -s show all tablespaces\n"
-		   "  -S show system objects too\n"
-		   "  -t TABLE   show info for named table\n"
-		   "  -U NAMEconnect as specified database user\n"
-		   "  -V, --version  output version information, then exit\n"
-		   "  -x extended (show additional columns)\n"
-		   "  -?, --help show this help, then exit\n"
+		   "  -f, --filenode FILENODEshow info for table with given file node\n"
+		   "  -i, --indexshow indexes and sequences too\n"
+		   "  -o, --oid=OID  show info for table with given OID\n"
+		   "  -q, --quietquiet (don't show headers)\n"
+		   "  -s, --tablespace   show all tablespaces\n"
+		   "  -S, --system 

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Fabien COELHO]

Hello Marina,

Detailed -r report. I understand from the doc that the retry number on 
the detailed per-statement report is to identify at what point errors 
occur? Probably this is more or less always at the same point on a 
given script, so that the most interesting feature is to report the 
number of retries at the script level.


This may depend on various factors.. for example:
[...]
   21.239   5  36  UPDATE xy3 SET y = y + :delta WHERE x 
= :x3;
   21.360   5  39  UPDATE xy2 SET y = y + :delta WHERE x 
= :x2;


Ok, not always the same point, and you confirm that it identifies where 
the error is raised which leads to a retry.


And you can always get the number of retries at the script level from the 
main report (if only one script is used) or from the report for each script 
(if multiple scripts are used).


Ok.

ISTM that "skipped" transactions are NOT "successful" so there are a 
problem with comments. I believe that your formula are probably right, 
it has more to do with what is "success". For cnt decomposition, ISTM 
that "other transactions" are really "directly successful 
transactions".


I agree with you, but I also think that skipped transactions should not be 
considered errors.


I'm ok with having a special category for them in the explanations, which 
is neither success nor error.



So we can write something like this:


All the transactions are divided into several types depending on their 
execution. Firstly, they can be divided into transactions that we started to 
execute, and transactions which were skipped (it was too late to execute 
them). Secondly, running transactions fall into 2 main types: is there any 
command that got a failure during the last execution of the transaction 
script or not? Thus


Here is an attempt at having a more precise and shorter version, not sure 
it is much better than yours, though:


"""
Transactions are counted depending on their execution and outcome. First
a transaction may have started or not: skipped transactions occur under 
--rate and --latency-limit when the client is too late to execute them. 
Secondly, a started transaction may ultimately succeed or fail on some 
error, possibly after some retries when --max-tries is not one. Thus

"""


the number of all transactions =
 skipped (it was too late to execute them)
 cnt (the number of successful transactions) +
 ecnt (the number of failed transactions).

A successful transaction can have several unsuccessful tries before a
successfull run. Thus

cnt (the number of successful transactions) =
 retried (they got a serialization or a deadlock failure(s), but were
  successfully retried from the very beginning) +
 directly successfull transactions (they were successfully completed on
the first try).


These above description is clearer for me.

I'd suggest to put "ANOTHER_SQL_FAILURE" as the last option, otherwise 
"another" does not make sense yet.


Maybe firstly put a general group, and then special cases?...


I understand it more as a catch all default "none of the above" case.

In TState, field "uint32 retries": maybe it would be simpler to count 
"tries", which can be compared directly to max tries set in the option?


If you mean retries in CState - on the one hand, yes, on the other hand, 
statistics always use the number of retries...


Ok.


The automaton skips to FAILURE on every possible error. I'm wondering 
whether it could do so only on SQL errors, because other fails will 
lead to ABORTED anyway? If there is no good reason to skip to FAILURE 
from some errors, I'd suggest to keep the previous behavior. Maybe the 
good reason is to do some counting, but this means that on eg 
metacommand errors now the script would loop over instead of aborting, 
which does not look like a desirable change of behavior.


Even in the case of meta command errors we must prepare for CSTATE_END_TX and 
the execution of the next script: if necessary, clear the conditional stack 
and rollback the current transaction block.


Seems ok.


commandFailed: I'm not thrilled by the added boolean, which is partially
redundant with the second argument.


Do you mean that it is partially redundant with the argument "cmd" and, for 
example, the meta commands errors always do not cause the abortions of the 
client?


Yes. And also I'm not sure we should want this boolean at all.


[...]
If in such cases one command is placed on several lines, ISTM that the code 
is more understandable if curly brackets are used...


Hmmm. Such basic style changes are avoided because they break 
backpatching, so we try to avoid gratuitous changes unless there is a 
strong added value, which does not seem to be the case here.


--
Fabien.

Re: ToDo: show size of partitioned table

[Mathias Brossard]

On Thu, Aug 16, 2018 at 12:46 AM Pavel Stehule 
wrote:

> čt 16. 8. 2018 v 5:52 odesílatel Mathias Brossard 
> napsal:
>
>> I do have a feedback on the implementation. The code tries to support
>> older PostgreSQL server versions when declarative partitions were not
>> supported before version 10 (relkind value of 'p'). Those versions will
>> never return any result from the query being built. So I would suggest an
>> early return from the function. The upside would be that the query building
>> would be simpler. I can make patch implementing that suggestion if you want.
>>
>
> This is question - maybe we can support older partitioning based on only
> inheritance - and the query can be more exact on PostgreSQL 10 and newer.
>
> Please, send any patch. You are welcome.
>

In my very humble opinion, I would restrict the definition of partitions to
declarative partitioning. My justification would be that partitions all use
inheritance, but not all inheritance is a partition (how would you handle
multiple inheritance).

See patch attached that fails (in a way similar to other features) when
connected to servers with version earlier than 10.0.

 Sincerely,
-- Mathias Brossard


psql-dP-10+only.patch
Description: Binary data

Re: TupleTableSlot abstraction

[Andres Freund]

Hi,

On 2018-08-17 12:10:20 +0530, Ashutosh Bapat wrote:
> We need to add LLVM code to fetch tts_flags and
> perform bit operation on it to get or set slow property. I haven't
> found any precedence for LLVM bit operations in postgresql's JIT code.

There are several, look for the infomask accesses in
slot_compiler_deform.

I'll try to do the adaption later today.

Greetings,

Andres Freund

Re: Doc patch: pg_upgrade page and checkpoint location consistency with replicas

[Paul Bonaud]

I shared the pach in plain textin the email body and figured out that
all other patches are submitted as an attachement. Sorry for that, here
is the patch attached to this email.

Thanks!
Paul

On 17/08/18 01:21, Paul Bonaud wrote:
> Hello,
> 
> Please find below a submission of a patch to the PostgreSQL documentation.
> 
> You can also find the patch in this git commit:
> https://gitlab.com/paulrbr/postgresql/commit/024d3870450df6dcdc69bddbe2de46084b73e3a2.diff
> 
> 
> commit 024d3870450df6dcdc69bddbe2de46084b73e3a2
> Author: Paul B 
> Date:   Thu Aug 16 18:25:22 2018 +0200
> 
> doc: Update pg_upgrade page while checking checkpoint locations
> 
> At the end of the previous step 7. you already stopped the primary
>  server. Which led PostgreSQL to issue a CHECKPOINT command.
> Aksi the end of the step 7. also states that "standby servers can
> remain running until a later step".
> 
> However if you keep your standby server running at this point and
> compare the Latest checkpoint location with your
> stopped primary you will never end up with matching values.
> 
> I found it confusing during my pg_upgrade tests as I tried multiple
> times to end up with the same latest checkpoint location value between
> my primary and standby nodes until I realised PostgreSQL was issuing a
> CHECKPOINT during shutdown which would obviously prevent that.
> 
> I reckon some clarification should be added to the documentation for
> that and that is why I propose this patch.
> 
> Please let me know if you want to phrase it differently or if I am
> missing something out.
> 
> Thank you!
> 
> diff --git a/doc/src/sgml/ref/pgupgrade.sgml
> b/doc/src/sgml/ref/pgupgrade.sgml
> index 6dafb404a11..d51146d641d 100644
> --- a/doc/src/sgml/ref/pgupgrade.sgml
> +++ b/doc/src/sgml/ref/pgupgrade.sgml
> @@ -326,7 +326,8 @@ NET STOP postgresql-&majorversion;
>   against the old primary and standby clusters.  Verify that the
>   Latest checkpoint location values match in all
> clusters.
>   (There will be a mismatch if old standby servers were shut down
> - before the old primary.)  Also, change wal_level to
> + before the old primary or if the old standby servers are still
> running.)
> + Also, change wal_level to
>   replica in the
> postgresql.conf file on the
>   new primary cluster.
>  
> 
> 
> ---
> Paul Bonaud
> 
diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index d51146d641d..6dafb404a11 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -326,8 +326,7 @@ NET STOP postgresql-&majorversion;
  against the old primary and standby clusters.  Verify that the
  Latest checkpoint location values match in all clusters.
  (There will be a mismatch if old standby servers were shut down
- before the old primary or if the old standby servers are still running.)
- Also, change wal_level to
+ before the old primary.)  Also, change wal_level to
  replica in the postgresql.conf file on the
  new primary cluster.
 


signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

On 17-08-2018 10:49, Fabien COELHO wrote:

Hello Marina,

Detailed -r report. I understand from the doc that the retry number 
on the detailed per-statement report is to identify at what point 
errors occur? Probably this is more or less always at the same point 
on a given script, so that the most interesting feature is to report 
the number of retries at the script level.


This may depend on various factors.. for example:
[...]
   21.239   5  36  UPDATE xy3 SET y = y + :delta 
WHERE x = :x3;
   21.360   5  39  UPDATE xy2 SET y = y + :delta 
WHERE x = :x2;


Ok, not always the same point, and you confirm that it identifies
where the error is raised which leads to a retry.


Yes, I confirm this. I'll try to write more clearly about this in the 
documentation...



So we can write something like this:


All the transactions are divided into several types depending on their 
execution. Firstly, they can be divided into transactions that we 
started to execute, and transactions which were skipped (it was too 
late to execute them). Secondly, running transactions fall into 2 main 
types: is there any command that got a failure during the last 
execution of the transaction script or not? Thus


Here is an attempt at having a more precise and shorter version, not
sure it is much better than yours, though:

"""
Transactions are counted depending on their execution and outcome. 
First

a transaction may have started or not: skipped transactions occur
under --rate and --latency-limit when the client is too late to
execute them. Secondly, a started transaction may ultimately succeed
or fail on some error, possibly after some retries when --max-tries is
not one. Thus
"""


Thank you!

I'd suggest to put "ANOTHER_SQL_FAILURE" as the last option, 
otherwise "another" does not make sense yet.


Maybe firstly put a general group, and then special cases?...


I understand it more as a catch all default "none of the above" case.


Ok!

commandFailed: I'm not thrilled by the added boolean, which is 
partially

redundant with the second argument.


Do you mean that it is partially redundant with the argument "cmd" 
and, for example, the meta commands errors always do not cause the 
abortions of the client?


Yes. And also I'm not sure we should want this boolean at all.


Perhaps we can use a separate function to print the messages about 
client's abortion, something like this (it is assumed that all abortions 
happen when processing SQL commands):


static void
clientAborted(CState *st, const char *message)
{
pgbench_error(...,
  "client %d aborted in command %d (SQL) of script 
%d; %s\n",
  st->id, st->command, st->use_file, message);
}

Or perhaps we can use a more detailed failure status so for each type of 
failure we always know the command name (argument "cmd") and whether the 
client is aborted. Something like this (but in comparison with the first 
variant ISTM overly complicated):


/*
 * For the failures during script execution.
 */
typedef enum FailureStatus
{
NO_FAILURE = 0,

/*
 * Failures in meta commands. In these cases the failed transaction is
 * terminated.
 */
META_SET_FAILURE,
META_SETSHELL_FAILURE,
META_SHELL_FAILURE,
META_SLEEP_FAILURE,
META_IF_FAILURE,
META_ELIF_FAILURE,

/*
	 * Failures in SQL commands. In cases of serialization/deadlock 
failures a
	 * failed transaction is re-executed from the very beginning if 
possible;

 * otherwise the failed transaction is terminated.
 */
SERIALIZATION_FAILURE,
DEADLOCK_FAILURE,
OTHER_SQL_FAILURE,  /* other failures in SQL 
commands that are not
 * listed by 
themselves above */

/*
 * Failures while processing SQL commands. In this case the client is
 * aborted.
 */
SQL_CONNECTION_FAILURE
} FailureStatus;


[...]
If in such cases one command is placed on several lines, ISTM that the 
code is more understandable if curly brackets are used...


Hmmm. Such basic style changes are avoided because they break
backpatching, so we try to avoid gratuitous changes unless there is a
strong added value, which does not seem to be the case here.


Ok!

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

libpq stricter integer parsing

[Fabien COELHO]

Follow up on a patch and discussion with Tom, currently integer parsing on 
keywords in libpq is quite loose, resulting in trailing garbage being 
ignored and allowing to hide bugs, eg:


  sh> psql "connect_timeout=2,port=5433"

The timeout is set to 2, and the port directive is silently ignored.
However, URL parsing is stricter, eg on "port".

The attached patch checks integer syntax errors and overflows, and report 
errors.


The pros is that it helps detect bugs. The cons is that some people may 
not want to know about these if it works in the end.


--
Fabien.diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 49de975e7f..94d114562a 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1579,6 +1579,16 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 
 

+
+   
+Integer values expected for keywords port,
+connect_timeout,
+keepalives_idle,
+keepalives_interval and
+keepalives_timeout are parsed strictly.
+Versions of libpq before
+PostgreSQL 12 accepted trailing garbage or overflows.
+   
   
  
 
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 9315a27561..15280dc208 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1586,6 +1586,35 @@ useKeepalives(PGconn *conn)
 	return val != 0 ? 1 : 0;
 }
 
+/*
+ * Parse integer, report any syntax error, and tell if all is well.
+ */
+static bool
+strict_atoi(const char *str, int *val, PGconn *conn, const char *context)
+{
+	char 	*endptr;
+
+	errno = 0;
+	*val = strtol(str, &endptr, 10);
+
+	if (errno != 0)
+	{
+		appendPQExpBuffer(&conn->errorMessage,
+		  libpq_gettext("invalid integer \"%s\" for keyword \"%s\": %s\n"),
+		  str, context, strerror(errno));
+		return false;
+	}
+	else if (*endptr != '\0')
+	{
+		appendPQExpBuffer(&conn->errorMessage,
+		  libpq_gettext("invalid integer \"%s\" for keyword \"%s\"\n"),
+		  str, context);
+		return false;
+	}
+
+	return true;
+}
+
 #ifndef WIN32
 /*
  * Set the keepalive idle timer.
@@ -1598,7 +1627,9 @@ setKeepalivesIdle(PGconn *conn)
 	if (conn->keepalives_idle == NULL)
 		return 1;
 
-	idle = atoi(conn->keepalives_idle);
+	if (!strict_atoi(conn->keepalives_idle, &idle, conn, "keepalives_idle"))
+		return 0;
+
 	if (idle < 0)
 		idle = 0;
 
@@ -1630,7 +1661,9 @@ setKeepalivesInterval(PGconn *conn)
 	if (conn->keepalives_interval == NULL)
 		return 1;
 
-	interval = atoi(conn->keepalives_interval);
+	if (!strict_atoi(conn->keepalives_interval, &interval, conn, "keepalives_interval"))
+		return 0;
+
 	if (interval < 0)
 		interval = 0;
 
@@ -1663,7 +1696,9 @@ setKeepalivesCount(PGconn *conn)
 	if (conn->keepalives_count == NULL)
 		return 1;
 
-	count = atoi(conn->keepalives_count);
+	if (!strict_atoi(conn->keepalives_count, &count, conn, "keepalives_count"))
+		return 0;
+
 	if (count < 0)
 		count = 0;
 
@@ -1697,13 +1732,17 @@ setKeepalivesWin32(PGconn *conn)
 	int			idle = 0;
 	int			interval = 0;
 
-	if (conn->keepalives_idle)
-		idle = atoi(conn->keepalives_idle);
+	if (conn->keepalives_idle &&
+		!strict_atoi(conn->keepalives_idle, &idle, conn, "keepalives_idle"))
+		return 0;
+
+	if (conn->keepalives_interval &&
+		!strict_atoi(conn->keepalives_interval, &interval, conn, "keepalives_interval"))
+		return 0;
+
 	if (idle <= 0)
 		idle = 2 * 60 * 60;		/* 2 hours = default */
 
-	if (conn->keepalives_interval)
-		interval = atoi(conn->keepalives_interval);
 	if (interval <= 0)
 		interval = 1;			/* 1 second = default */
 
@@ -1784,7 +1823,9 @@ connectDBStart(PGconn *conn)
 			thisport = DEF_PGPORT;
 		else
 		{
-			thisport = atoi(ch->port);
+			if (!strict_atoi(ch->port, &thisport, conn, "port"))
+goto connect_errReturn;
+
 			if (thisport < 1 || thisport > 65535)
 			{
 appendPQExpBuffer(&conn->errorMessage,
@@ -1916,7 +1957,9 @@ connectDBComplete(PGconn *conn)
 	 */
 	if (conn->connect_timeout != NULL)
 	{
-		timeout = atoi(conn->connect_timeout);
+		if (!strict_atoi(conn->connect_timeout, &timeout, conn, "connect_timeout"))
+			return 0;
+
 		if (timeout > 0)
 		{
 			/*
@@ -1927,6 +1970,8 @@ connectDBComplete(PGconn *conn)
 			if (timeout < 2)
 timeout = 2;
 		}
+		else /* negative means 0 */
+			timeout = 0;
 	}
 
 	for (;;)

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Fabien COELHO]

commandFailed: I'm not thrilled by the added boolean, which is partially
redundant with the second argument.


Do you mean that it is partially redundant with the argument "cmd" and, 
for example, the meta commands errors always do not cause the abortions of 
the client?


Yes. And also I'm not sure we should want this boolean at all.


Perhaps we can use a separate function to print the messages about client's 
abortion, something like this (it is assumed that all abortions happen when 
processing SQL commands):


static void
clientAborted(CState *st, const char *message)


Possibly.

Or perhaps we can use a more detailed failure status so for each type of 
failure we always know the command name (argument "cmd") and whether the 
client is aborted. Something like this (but in comparison with the first 
variant ISTM overly complicated):


I agree., I do not think that it would be useful given that the same thing 
is done on all meta-command error cases in the end.


--
Fabien.

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

On 17-08-2018 14:04, Fabien COELHO wrote:

...
Or perhaps we can use a more detailed failure status so for each type 
of failure we always know the command name (argument "cmd") and 
whether the client is aborted. Something like this (but in comparison 
with the first variant ISTM overly complicated):


I agree., I do not think that it would be useful given that the same
thing is done on all meta-command error cases in the end.


Ok!

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: docs: note ownership requirement for refreshing materialized views

[Dave Cramer]

Dave Cramer


On Thu, 16 Aug 2018 at 18:27, Jonathan S. Katz <
jonathan.k...@excoventures.com> wrote:

>
> On Aug 16, 2018, at 1:05 AM, Jonathan S. Katz <
> jonathan.k...@excoventures.com> wrote:
>
>
> On Aug 15, 2018, at 9:15 PM, Michael Paquier  wrote:
>
> On Wed, Aug 15, 2018 at 09:06:34PM -0400, Jonathan S. Katz wrote:
>
> I played around with this feature a bit and did see this was the case.
> Also while playing around I noticed the error message was as such:
>
> test=> REFRESH MATERIALIZED VIEW blah;
> ERROR: must be owner of relation blah
>
> But it’s not a relation, it’s a materialized view. I attached a patch
> that I think should fix this. Kudos to Dave Cramer who was
> sitting next to me helping me to locate files and confirm assumptions.
>
>
> A relation may be a materialized view, no?  The ACL check happens in
> RangeVarCallbackOwnsTable by the way (look at ExecRefreshMatView in
> matview.c).
>
>
> Comment on the RangeVarCallbackOwnsTable func (abbr):
>
>/*
> * This is intended as a callback for RangeVarGetRelidExtended().  It
> allows
> * the relation to be locked only if (1) it's a plain table,
> materialized
> * view, or TOAST table and (2) the current user is the owner (or the
> * superuser).  This meets the permission-checking needs of CLUSTER,
> REINDEX
> * TABLE, and REFRESH MATERIALIZED VIEW; we expose it here so that it
> can be
> * used by all.
> */
>
> So it’s sharing the permission checking needs amongst all of those
> commands.
>
> As a user I could be confused if I saw the above error message, esp.
> because
> the behavior of REFRESH .. is specific to materialized views.
>
>
> With encouragement from Dave, let me demonstrate what the proposed patch
> does to fix the behavior. The steps, running from my “jkatz” user:
>
> CREATE ROLE bar LOGIN;
> CREATE TABLE a (x int);
> CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
> \c - bar
> REFRESH MATERIALIZED VIEW b;
> ERROR:  must be owner of materialized view b
>
> vs. the existing error message which I posted further upthread.
>
> Thanks,
>
> Jonathan
>

So it seems this patch is being ignored in this thread.
AFAICT, in aclcheck_error() the only way to get to the following sub case
in the case ACLCHECK_NOT_OWNER

case OBJECT_MATVIEW:
msg = gettext_noop("must be owner of materialized view %s");
break;

is if the patch is applied ?

Regards,

Dave

Re: docs: note ownership requirement for refreshing materialized views

[Dian Fay]

Jonathan's patch seems like a good idea to me from a user POV, but then 
I just showed up the other day so I don't really have anything of 
substance to add.


On 8/17/18 9:08 AM, Dave Cramer wrote:


Dave Cramer


On Thu, 16 Aug 2018 at 18:27, Jonathan S. Katz 
> wrote:




On Aug 16, 2018, at 1:05 AM, Jonathan S. Katz
mailto:jonathan.k...@excoventures.com>> wrote:



On Aug 15, 2018, at 9:15 PM, Michael Paquier
mailto:mich...@paquier.xyz>> wrote:

On Wed, Aug 15, 2018 at 09:06:34PM -0400, Jonathan S. Katz wrote:

I played around with this feature a bit and did see this was
the case.
Also while playing around I noticed the error message was as such:

test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blah

But it’s not a relation, it’s a materialized view. I attached a
patch
that I think should fix this. Kudos to Dave Cramer who was
sitting next to me helping me to locate files and confirm
assumptions.


A relation may be a materialized view, no?  The ACL check happens in
RangeVarCallbackOwnsTable by the way (look at ExecRefreshMatView in
matview.c).


Comment on the RangeVarCallbackOwnsTable func (abbr):

   /*
* This is intended as a callback for
RangeVarGetRelidExtended().  It allows
* the relation to be locked only if (1) it's a plain table,
materialized
* view, or TOAST table and (2) the current user is the owner
(or the
* superuser).  This meets the permission-checking needs of
CLUSTER, REINDEX
* TABLE, and REFRESH MATERIALIZED VIEW; we expose it here so
that it can be
* used by all.
*/

So it’s sharing the permission checking needs amongst all of
those commands.

As a user I could be confused if I saw the above error message,
esp. because
the behavior of REFRESH .. is specific to materialized views.


With encouragement from Dave, let me demonstrate what the proposed
patch
does to fix the behavior. The steps, running from my “jkatz” user:

CREATE ROLE bar LOGIN;
CREATE TABLE a (x int);
CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
\c - bar
REFRESH MATERIALIZED VIEW b;
ERROR:  must be owner of materialized view b

vs. the existing error message which I posted further upthread.

Thanks,

Jonathan


So it seems this patch is being ignored in this thread.
AFAICT, in aclcheck_error() the only way to get to the following sub 
case in the case ACLCHECK_NOT_OWNER


case OBJECT_MATVIEW:
msg = gettext_noop("must be owner of materialized view %s");
break;

is if the patch is applied ?

Regards,

Dave

Re: Fix help option of contrib/oid2name

[Alvaro Herrera]

On 2018-Aug-17, Tatsuro Yamada wrote:

>   only oid2name
>   - Replace -H with -h

I think this one is a bad idea, as it'll break scripts.

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

Re: docs: note ownership requirement for refreshing materialized views

[Tom Lane]

Dave Cramer  writes:
> So it seems this patch is being ignored in this thread.

Well, Jonathan did kind of hijack what appears to be a thread about
documentation (with an already-committed fix).

I'd suggest reposting that patch in its own thread and adding it to
the next CF.

regards, tom lane

Re: Facility for detecting insecure object naming

[Bruce Momjian]

On Thu, Aug 16, 2018 at 10:58:21PM -0400, Chapman Flack wrote:
> On 08/16/18 21:31, Bruce Momjian wrote:
> 
> > I understand you don't like that a search_path changed by a function is
> > passed down to functions it calls, but what would you like the system to
> > use as a search path for called functions?  The search path of the
> > session?
> 
> This is beginning to sound like an exercise in /emulating/ lexical scoping
> in a system that can run bits of code supplied in many different PLs, not
> all of which can necessarily participate in "real" lexical scoping.
> 
> It could look like another flavor of setting a GUC. There's SET SESSION,
> SET LOCAL, and maybe now SET LEXICAL. The key to the emulation is to
> temporarily unstack the SET LEXICAL when the function it "lexically"
> applies to makes calls to other functions. And restack it when those
> other functions return.

Right.  My point is that people don't like dynamic scoping in Perl
because it overrides the lexical scoping implicit in the source code
structure.  For Postgres objects, like functions, there is no implicit
scope --- each object is defined and managed on its own.  So, the
problem isn't that dynamic scoping is overriding implicit scoping, but
that we only have dynamic scoping, and there is no implicit scoping to
override.

So, to move this forward, we have to have some way of defining implicit
scoping.  It could be:

o  other objects in the same schema
o  owned by the same user
o  a member of the same extension
o  a SET search_path in the function
o  the search_path at object creation time
o  objects referenced at object creation time

(I guess Oracle's package syntax helps them here.)  You have to define
some implicit scoping so you don't have to rely on dynamic scoping.

Two of Robert's problem cases were expression indexes that are later
reindexed, and calling an extension that wants to call functions from the
same extension, even if the search_path does not include the extension's
schema.  So, which implicit scoping rules would fix those problems?

In addition, you have people adding, updating, and removing objects
while these functions are required to still run properly.  That is
something that also doesn't typically happen in a Perl program, because
the source files or package APIs are assumed to be tightly managed.

Also, with Perl, you have a layer of objects, from built-in ones, to
packages, to application-level objects.  With Postgres, they are all
thrown together into the same tables, and there is no filtering of
scopes.

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

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

Re: Fix help option of contrib/oid2name

[Tom Lane]

Alvaro Herrera  writes:
> On 2018-Aug-17, Tatsuro Yamada wrote:
>> only oid2name
>> - Replace -H with -h

> I think this one is a bad idea, as it'll break scripts.

Well, we can't remove the -H option, for that reason.  But I think
we could get away with repurposing -h to also mean "--host", rather
than "--help" as it is now.  Seems unlikely that any scripts are
depending on it to mean --help, nor are users likely to expect that
when none of our other programs treat it that way.

regards, tom lane

Re: Pre-v11 appearances of the word "procedure" in v11 docs

[Peter Eisentraut]

Attached are my proposed patches.  The first is the documentation
change, which basically just substitutes the words, with some occasional
rephrasing.  And then patches to extend the syntaxes of CREATE OPERATOR,
CREATE TRIGGER, and CREATE EVENT TRIGGER to accept FUNCTION in place of
PROCEDURE.  I decided to do that because with the adjustments from the
first patch, the documentation had become comically inconsistent in some
places and it was easier to just fix the underlying problem than to
explain the reasons for the inconsistencies everywhere.  I didn't go
around change all the commands in contrib modules etc. to keep the patch
size under control.  This could perhaps be done later.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 142d34b7532fdd8e439bdf21b00983c13f4325a5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 15 Aug 2018 17:01:39 +0200
Subject: [PATCH 1/3] doc: Update uses of the word "procedure"

Historically, the term procedure was used as a synonym for function in
Postgres/PostgreSQL.  Now we have procedures as separate objects from
functions, so we need to clean up the documentation to not mix those
terms.

In particular, mentions of "trigger procedures" are changed to "trigger
functions", and access method "support procedures" are changed to
"support functions".  (The latter already used FUNCTION in the SQL
syntax anyway.)  Also, the terminology in the SPI chapter has been
cleaned up.

A few tests, examples, and code comments are also adjusted to be
consistent with documentation changes, but not everything.
---
 doc/src/sgml/brin.sgml| 52 ++--
 doc/src/sgml/catalogs.sgml| 20 ++---
 doc/src/sgml/event-trigger.sgml   |  6 +-
 doc/src/sgml/func.sgml|  2 +-
 doc/src/sgml/plhandler.sgml   |  2 +-
 doc/src/sgml/plperl.sgml  |  2 +-
 doc/src/sgml/plpgsql.sgml | 34 
 doc/src/sgml/pltcl.sgml   | 40 -
 doc/src/sgml/ref/alter_opfamily.sgml  |  4 +-
 doc/src/sgml/ref/create_language.sgml |  2 +-
 doc/src/sgml/ref/create_opclass.sgml  |  6 +-
 doc/src/sgml/ref/create_operator.sgml |  8 +-
 doc/src/sgml/ref/create_trigger.sgml  |  2 +-
 doc/src/sgml/spi.sgml | 83 +--
 doc/src/sgml/xindex.sgml  |  2 +-
 doc/src/sgml/xplang.sgml  |  4 +-
 src/backend/access/gin/ginvalidate.c  |  2 +-
 src/backend/access/gist/gistvalidate.c|  2 +-
 src/backend/access/hash/hashutil.c|  4 +-
 src/backend/access/hash/hashvalidate.c|  4 +-
 src/backend/access/spgist/spgvalidate.c   |  2 +-
 src/backend/commands/opclasscmds.c| 30 +++
 src/bin/psql/describe.c   |  2 +-
 src/include/access/hash.h | 12 +--
 src/test/regress/expected/alter_generic.out   | 16 ++--
 src/test/regress/expected/create_operator.out |  6 +-
 src/test/regress/sql/create_operator.sql  |  6 +-
 27 files changed, 174 insertions(+), 181 deletions(-)

diff --git a/doc/src/sgml/brin.sgml b/doc/src/sgml/brin.sgml
index f02e061bc1..f47e1968a4 100644
--- a/doc/src/sgml/brin.sgml
+++ b/doc/src/sgml/brin.sgml
@@ -537,7 +537,7 @@ Extensibility
 } BrinOpcInfo;
 
   
BrinOpcInfo.oi_opaque can 
be used by the
-  operator class routines to pass information between support procedures
+  operator class routines to pass information between support functions
   during an index scan.
  
 
@@ -587,27 +587,27 @@ Extensibility
   defined by the user for other data types using equivalent definitions,
   without having to write any source code; appropriate catalog entries being
   declared is enough.  Note that assumptions about the semantics of operator
-  strategies are embedded in the support procedures' source code.
+  strategies are embedded in the support functions' source code.
  
 
  
   Operator classes that implement completely different semantics are also
-  possible, provided implementations of the four main support procedures
+  possible, provided implementations of the four main support functions
   described above are written.  Note that backwards compatibility across major
-  releases is not guaranteed: for example, additional support procedures might
+  releases is not guaranteed: for example, additional support functions might
   be required in later releases.
  
 
  
   To write an operator class for a data type that implements a totally
-  ordered set, it is possible to use the minmax support procedures
+  ordered set, it is possible to use the minmax support functions
   alongside the corresponding operators, as shown in
   .
-  All operator class members (procedures and operators) are mandatory.
+  All operator class members (functions and operato

Re: Memory leak with CALL to Procedure with COMMIT.

[Peter Eisentraut]

On 16/08/2018 19:26, Tom Lane wrote:
>> When a CALL has output parameters, the portal uses the strategy
>> PORTAL_UTIL_SELECT instead of PORTAL_MULTI_QUERY.  Using
>> PORTAL_UTIL_SELECT causes the portal's snapshot to be registered with
>> the current resource owner (portal->holdSnapshot).  I'm not sure why
>> this is done for one kind of portal strategy but not the other.

> I'm a bit confused by that statement.  AFAICS, for both PortalRunUtility
> and PortalRunMulti, setHoldSnapshot is only passed as true by
> FillPortalStore, so registering the snapshot should happen (or not) the
> same way for either portal execution strategy.  What scenarios are you
> comparing here, exactly?

The call to PortalRunMulti() in FillPortalStore() is for
PORTAL_ONE_RETURNING and PORTAL_ONE_MOD_WITH, not for
PORTAL_MULTI_QUERY.  For PORTAL_MULTI_QUERY, FillPortalStore() isn't
called; PortalRun() calls PortalRunMulti() directly with setHoldSnapshot
= false.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH] Improve geometric types

[Tomas Vondra]

Hi,

the buildfarm seems to be mostly happy so far, so I've taken a quick
look at the remaining two parts. The patches still apply, but I'm
getting plenty of failures in regression tests, due to 0.0 being
replaced by -0.0.

This reminds me 74294c7301, except that these patches don't seem to
remove any such checks by mistake. Instead it seems to be caused by
simply switching to float8_ methods. The attached patch fixes the issue
for me, although I'm not claiming it's the right way to fix it.

Another thing I noticed is the last few lines from line_interpt_line are
actually unreachable, because there's now 'else return false' branch.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index e0a9a0fa4f..97b3349ff8 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -1251,6 +1251,14 @@ line_interpt_line(Point *result, LINE *l1, LINE *l2)
 	   float8_mi(float8_mul(l1->A, l2->B),
  float8_mul(l2->A, l1->B)));
 		y = float8_div(-float8_pl(float8_mul(l1->A, x), l1->C), l1->B);
+
+		/* on some platforms, the preceding expression tends to produce -0 */
+		if (x == 0.0)
+			x = 0.0;
+
+		/* on some platforms, the preceding expression tends to produce -0 */
+		if (y == 0.0)
+			y = 0.0;
 	}
 	else if (!FPzero(l2->B))
 	{
@@ -1262,6 +1270,14 @@ line_interpt_line(Point *result, LINE *l1, LINE *l2)
 	   float8_mi(float8_mul(l2->A, l1->B),
  float8_mul(l1->A, l2->B)));
 		y = float8_div(-float8_pl(float8_mul(l2->A, x), l2->C), l2->B);
+
+		/* on some platforms, the preceding expression tends to produce -0 */
+		if (x == 0.0)
+			x = 0.0;
+
+		/* on some platforms, the preceding expression tends to produce -0 */
+		if (y == 0.0)
+			y = 0.0;
 	}
 	else
 		return false;
@@ -1798,6 +1814,12 @@ point_send(PG_FUNCTION_ARGS)
 static inline void
 point_construct(Point *result, float8 x, float8 y)
 {
+	if (x == 0.0)
+		x = 0.0;
+
+	if (y == 0.0)
+		y = 0.0;
+
 	result->x = x;
 	result->y = y;
 }

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Alexander Korotkov]

On Thu, Aug 16, 2018 at 2:16 PM Alexander Korotkov
 wrote:
> On Tue, Aug 14, 2018 at 12:05 PM Masahiko Sawada  
> wrote:
> >
> > On Wed, Feb 28, 2018 at 11:24 PM, Ivan Kartyshov
> >  wrote:
> > > The main goal of my changes is to let long read-only transactions run on
> > > replica if hot_standby_feedback is turned on.
> >
> > FWIW the idea proposed on the thread[1], which allows us to disable
> > the heap truncation by vacuum per tables, might help this issue. Since
> > the original problem on that thread is a performance problem an
> > alternative proposal would be better, but I think the way would be
> > helpful for this issue too and is simple. A downside of that idea is
> > that there is additional work for user to configure the reloption to
> > each tables.
>
> Thank you for pointing this thread.  It's possible to cope this
> downside to introduce GUC which would serve as default, when reloption
> is not defined.  But real downside is inability to truncate the heap.
> So, options are good if they provide workaround for users, but that
> shouldn't stop us from fixing issues around heap truncation.

So, do we have any objections to committing this?

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Andres Freund]

Hi,

On 2018-08-17 18:00:20 +0300, Alexander Korotkov wrote:
> So, do we have any objections to committing this?

I think this needs more review by other senior hackers in the community.

Greetings,

Andres Freund

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Tom Lane]

Andres Freund  writes:
> On 2018-08-17 18:00:20 +0300, Alexander Korotkov wrote:
>> So, do we have any objections to committing this?

> I think this needs more review by other senior hackers in the community.

TBH it sounds like a horrible hack.  Disable vacuum truncation?
That can't be a good idea.  If it were something we were doing
behind the scenes as a temporary expedient, maybe we could hold
our noses and do it for awhile.  But once you introduce a reloption
based on this, you can't ever get rid of it.

I think we need to spend more effort looking for a less invasive,
less compromised solution.

regards, tom lane

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Andres Freund]

On 2018-08-17 11:35:40 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2018-08-17 18:00:20 +0300, Alexander Korotkov wrote:
> >> So, do we have any objections to committing this?
> 
> > I think this needs more review by other senior hackers in the community.
> 
> TBH it sounds like a horrible hack.  Disable vacuum truncation?

There's another patch, which I thought Alexander was referring to, that
does something a bit smarger.  On a super short skim it seems to
introduce a separate type of AEL lock that's not replicated, by my
reading?

Greetings,

Andres Freund

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Alexander Korotkov]

On Fri, Aug 17, 2018 at 6:41 PM Andres Freund  wrote:
> On 2018-08-17 11:35:40 -0400, Tom Lane wrote:
> > Andres Freund  writes:
> > > On 2018-08-17 18:00:20 +0300, Alexander Korotkov wrote:
> > >> So, do we have any objections to committing this?
> >
> > > I think this needs more review by other senior hackers in the community.
> >
> > TBH it sounds like a horrible hack.  Disable vacuum truncation?
>
> There's another patch, which I thought Alexander was referring to, that
> does something a bit smarger.  On a super short skim it seems to
> introduce a separate type of AEL lock that's not replicated, by my
> reading?

Yes, that's correct.  On standby read-only queries can tolerate
concurrent heap truncation.  So, there is no point to replicate AEL to
standby.  Previous versions of patch tries to do that by introducing
some flag.  But it appears that correct way to do this is just new
non-replicated type of AEL lock.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Slotification of partition tuple conversion

[Amit Khandekar]

Hi,

In [1] , it was shown that the partition tuples are needlessly formed
and deformed during tuple conversion (do_convert_tuple), when the same
operation can be done using tuple slots. This is because the input
slot might already have a deformed tuple.

Attached is a patch tup_convert.patch that does the conversion
directly using input and output tuple slots. This patch is to be
applied on an essential patch posted by Amit Langote in [1] that
arranges for dedicated per-partition slots rather than changing
tupdesc of a single slot. I have rebased that patch from [1] and
attached it here (
Allocate-dedicated-slots-of-partition-tuple_amitlan_rebased.patch).

In tup_convert.patch, wherever ConvertPartitionTupleSlot() and
do_convert_tuple() are used to convert partition tuples, I have
replaced them by a new function ConvertTupleSlot().

ConvertPartitionTupleSlot() is exclusively for conversion of
partition-to-parent and vice versa, so I got rid of this.
do_convert_tuple() seems to be used for tuples belonging to
non-partition tables as well, so I have left such calls untouched. May
be we can "slotify" these tuple conversions later as a separate task.

[1] 
https://www.postgresql.org/message-id/e4f9d743-cd4b-efb0-7574-da21d86a7f36%40lab.ntt.co.jp


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company


tup_convert.patch
Description: Binary data


Allocate-dedicated-slots-of-partition-tuple_amitlan_rebased.patch
Description: Binary data

Re: partitioning - changing a slot's descriptor is expensive

[Amit Khandekar]

On 29 June 2018 at 11:53, Amit Langote  wrote:
> Other issues that you mentioned, such as needless heap_tuple_deform/form
> being invoked, seem less localized (to me) than this particular issue, so
> I created a patch for just this, which is attached with this email.  I'm
> thinking about how to fix the other issues, but will need a bit more time.

I have started a new thread on these tuple forming/deforming issues
[1], and posted there a patch that is to be applied over your
Allocate-dedicated-slots-of-partition-tuple.patch (which I rebased and
posted in that thread) :

[1] 
https://www.postgresql.org/message-id/CAJ3gD9fR0wRNeAE8VqffNTyONS_UfFPRpqxhnD9Q42vZB%2BJvpg%40mail.gmail.com


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Re: [PATCH] Improve geometric types

[Emre Hasegeli]

> the buildfarm seems to be mostly happy so far, so I've taken a quick
> look at the remaining two parts. The patches still apply, but I'm
> getting plenty of failures in regression tests, due to 0.0 being
> replaced by -0.0.

I think we are better off fixing them locally at the moment like your
patch does.  We should consider to eliminate -0 globally for all
floating point based datatypes later.  I simplified and incorporated
your change to line_interpt_line() into mine.

I am not sure about normalising -0s on point_construct().  We
currently allow points to be initialized with -0s.  I think it is fair
for us to return -0 when -x and 0 are multiplied.  That is the current
behavior and the behavior of the float datatypes.  I adjusted the
results of the new regression tests accordingly.

> Another thing I noticed is the last few lines from line_interpt_line are
> actually unreachable, because there's now 'else return false' branch.

Which lines do you mean exactly?  I don't see any being unreachable.


0001-line-fixes-v14.patch
Description: Binary data


0002-geo-tests-v10.patch.gz
Description: GNU Zip compressed data

Re: Index Skip Scan

[Jesper Pedersen]

Hi Peter,

On 08/16/2018 03:48 PM, Peter Geoghegan wrote:

On Wed, Aug 15, 2018 at 11:22 PM, Thomas Munro
 wrote:

* groups and certain aggregates (MIN() and MAX() of suffix index
columns within each group)
* index scans where the scan key doesn't include the leading columns
(but you expect there to be sufficiently few values)
* merge joins (possibly the trickiest and maybe out of range)


FWIW, I suspect that we're going to have the biggest problems in the
optimizer. It's not as if ndistinct is in any way reliable. That may
matter more on average than it has with other path types.



Thanks for sharing this; it is very useful to know.

Best regards,
 Jesper

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 08/17/2018 06:40 PM, Emre Hasegeli wrote:
>> the buildfarm seems to be mostly happy so far, so I've taken a quick
>> look at the remaining two parts. The patches still apply, but I'm
>> getting plenty of failures in regression tests, due to 0.0 being
>> replaced by -0.0.
> 
> I think we are better off fixing them locally at the moment like your
> patch does.  We should consider to eliminate -0 globally for all
> floating point based datatypes later.  I simplified and incorporated
> your change to line_interpt_line() into mine.
> 
> I am not sure about normalising -0s on point_construct().  We
> currently allow points to be initialized with -0s.  I think it is fair
> for us to return -0 when -x and 0 are multiplied.  That is the current
> behavior and the behavior of the float datatypes.  I adjusted the
> results of the new regression tests accordingly.
> 

Hmmm, I need to think about that a bit more.

BTW how did we end up with the regression differences? Presumably you've
tried that on your machine and it passed. So if we adjust the expected
file, won't it fail on some other machines?

>> Another thing I noticed is the last few lines from line_interpt_line are
>> actually unreachable, because there's now 'else return false' branch.
> 
> Which lines do you mean exactly?  I don't see any being unreachable.
> 

Apologies, I got confused - there are no unreachable lines.


-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Tom Lane]

Alexander Korotkov  writes:
> On Fri, Aug 17, 2018 at 6:41 PM Andres Freund  wrote:
>> There's another patch, which I thought Alexander was referring to, that
>> does something a bit smarger.  On a super short skim it seems to
>> introduce a separate type of AEL lock that's not replicated, by my
>> reading?

> Yes, that's correct.  On standby read-only queries can tolerate
> concurrent heap truncation.

Uh, what???

regards, tom lane

Re: Index Skip Scan

[Peter Geoghegan]

On Thu, Aug 16, 2018 at 4:10 PM, Thomas Munro
 wrote:
> Can you give an example of problematic ndistinct underestimation?

Yes. See 
https://postgr.es/m/cakuk5j12qokfh88tqz-ojmsibg2qyjm7k7hlnbyi3ze+y5b...@mail.gmail.com,
for example. That's a complaint about an underestimation specifically.

This seems to come up about once every 3 years, at least from my
perspective. I'm always surprised that ndistinct doesn't get
implicated in bad query plans more frequently.

> I suppose you might be able to defend against that in the executor: if
> you find that you've done an unexpectedly high number of skips, you
> could fall back to regular next-tuple mode.  Unfortunately that's
> require the parent plan node to tolerate non-unique results.

I like the idea of dynamic fallback in certain situations, but the
details always seem complicated.

-- 
Peter Geoghegan

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Alexander Korotkov]

On Fri, Aug 17, 2018 at 8:38 PM Tom Lane  wrote:
> Alexander Korotkov  writes:
> > On Fri, Aug 17, 2018 at 6:41 PM Andres Freund  wrote:
> >> There's another patch, which I thought Alexander was referring to, that
> >> does something a bit smarger.  On a super short skim it seems to
> >> introduce a separate type of AEL lock that's not replicated, by my
> >> reading?
>
> > Yes, that's correct.  On standby read-only queries can tolerate
> > concurrent heap truncation.
>
> Uh, what???

VACUUM truncates heap relation only after deletion of all the tuples
from tail pages.  So, on standby heap truncation record would be
replayed only after heap tuples deletion records are replayed.
Therefore, if query on standby should see some of those tuples, WAL
replay stop (or query cancel) should happened before corresponding
tuples being deleted by our recovery conflict with snapshot logic.
When we're going to replay heap truncation record, no query should see
records in the tail pages.

And in md.c we already have logic to return zeroed pages, when trying
to read past relation in recovery.

/*
 * Short read: we are at or past EOF, or we read a partial block at
 * EOF.  Normally this is an error; upper levels should never try to
 * read a nonexistent block.  However, if zero_damaged_pages is ON or
 * we are InRecovery, we should instead return zeroes without
 * complaining.  This allows, for example, the case of trying to
 * update a block that was later truncated away.
 */
if (zero_damaged_pages || InRecovery)
MemSet(buffer, 0, BLCKSZ);
else
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
 errmsg("could not read block %u in file \"%s\":
read only %d of %d bytes",
blocknum, FilePathName(v->mdfd_vfd),
nbytes, BLCKSZ)));

But, I'm concerned if FileSeek() could return error.  And also what
_mdfd_getseg() would do on truncated segment.  It seems that in
recovery, it will automatically extend the relation.  That
unacceptable for this purpose.  So, sorry for bothering, this patch
definitely needs to be revised.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [PATCH] Improve geometric types

[Emre Hasegeli]

> BTW how did we end up with the regression differences? Presumably you've
> tried that on your machine and it passed. So if we adjust the expected
> file, won't it fail on some other machines?

I had another patch to check for -0 inside float{4,8}_{div,mul}().  I
dropped it on the last set of patches, so the tests were broken.  I
get -0 as a result of -x * 0 both on Mac and Linux.

Performance improvements for src/port/snprintf.c

[Tom Lane]

Over in the what-about-%m thread, we speculated about replacing the
platform's *printf functions if they didn't support %m, which would
basically mean using src/port/snprintf.c on all non-glibc platforms,
rather than only on Windows as happens right now (ignoring some
obsolete platforms with busted snprintf's).

I've been looking into the possible performance consequences of that,
in particular comparing snprintf.c to the library versions on macOS,
FreeBSD, OpenBSD, and NetBSD.  While it held up well in simpler cases,
I noted that it was significantly slower on long format strings, which
I traced to two separate problems:

1. Our implementation always scans the format string twice, so that it
can sort out argument-ordering options (%n$).  Everybody else is bright
enough to do that only for formats that actually use %n$, and it turns
out that it doesn't really cost anything extra to do so: you can just
perform the extra scan when and if you first find a dollar specifier.
(Perhaps there's an arguable downside for this, with invalid format
strings that have non-dollar conversion specs followed by dollar ones:
with this approach we might fetch some arguments before realizing that
the format is broken.  But a wrong format can cause indefinitely bad
results already, so that seems like a pretty thin objection to me,
especially if all other implementations share the same hazard.)

2. Our implementation is shoving simple data characters in the format
out to the result buffer one at a time.  More common is to skip to the
next % as fast as possible, and then dump anything skipped over using
the string-output code path, reducing the overhead of buffer overrun
checking.

The attached patch fixes both of those things, and also does some
micro-optimization hacking to avoid loops around dopr_outch() as well
as unnecessary use of pass-by-ref arguments.  This version stacks up
pretty well against all the libraries I compared it to.  The remaining
weak spot is that floating-point conversions are consistently 30%-50%
slower than the native libraries, which is not terribly surprising
considering that our implementation involves calling the native sprintf
and then massaging the result.  Perhaps there's a way to improve that
without writing our own floating-point conversion code, but I'm not
seeing an easy way offhand.  I don't think that's a showstopper though.
This code is now faster than the native code for very many other cases,
so on average it should cause no real performance problem.

I've attached both the patch and a simple performance testbed in case
anybody wants to do their own measurements.  For reference's sake,
these are the specific test cases I looked at:

snprintf(buffer, sizeof(buffer),
 "%2$.*3$f %1$d\n",
 42, 123.456, 2);

snprintf(buffer, sizeof(buffer),
 "%.*g", 15, 123.456);

snprintf(buffer, sizeof(buffer),
 "%d %d", 15, 16);

snprintf(buffer, sizeof(buffer),
 "%10d", 15);

snprintf(buffer, sizeof(buffer),
 "%s",
 
"0123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890");

snprintf(buffer, sizeof(buffer),
 "%d 
0123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890",

snprintf(buffer, sizeof(buffer),
 "%1$d 
0123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890",
 42);

A couple of other notes of interest:

* The skip-to-next-% searches could alternatively be implemented with
strchr(), although then you need a strlen() call if there isn't another %.
glibc's version of strchr() is fast enough to make that a win, but since
we're not contemplating using this atop glibc, that's not a case we care
about.  On other platforms the manual loop mostly seems to be faster.

* NetBSD seems to have a special fast path for the case that the format
string is exactly "%s".  I did not adopt that idea here, reasoning that
checking for it would add overhead to all other cases, making it probably
a net loss overall.  I'm prepared to listen to arguments otherwise,
though.  It is a common case, I just doubt it's common enough (and
other library authors seem to agree).

I'll add this to the upcoming CF.

regards, tom lane

diff --git a/src/port/snprintf.c b/src/port/snprintf.c
index 851e2ae..211ff1b 100644
*** a/src/port/snprintf.c
--- b/src/port/snprintf.c
*** flushbuffer(PrintfTarget *tar

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 08/17/2018 08:24 PM, Emre Hasegeli wrote:
>> BTW how did we end up with the regression differences? Presumably you've
>> tried that on your machine and it passed. So if we adjust the expected
>> file, won't it fail on some other machines?
> 
> I had another patch to check for -0 inside float{4,8}_{div,mul}().  I
> dropped it on the last set of patches, so the tests were broken.  I
> get -0 as a result of -x * 0 both on Mac and Linux.
> 

OK, that explains is. I won't have time to get this committed before CF
2018-09, but I'll pick it up in September.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Tom Lane]

Alexander Korotkov  writes:
> On Fri, Aug 17, 2018 at 8:38 PM Tom Lane  wrote:
>> Alexander Korotkov  writes:
>>> Yes, that's correct.  On standby read-only queries can tolerate
>>> concurrent heap truncation.

>> Uh, what???

> VACUUM truncates heap relation only after deletion of all the tuples
> from tail pages.

Right; the problem I'm worried about is possible accesses to
already-removed pages by concurrent scans.

> And in md.c we already have logic to return zeroed pages, when trying
> to read past relation in recovery.

But then you are injecting bad pages into the shared buffer arena.
In any case, depending on that behavior seems like a bad idea, because
it's a pretty questionable kluge in itself.

Another point is that the truncation code attempts to remove all
to-be-truncated-away pages from the shared buffer arena, but that only
works if nobody else is loading such pages into shared buffers
concurrently.  In the presence of concurrent scans, we might be left
with valid-looking buffers for pages that have been truncated away
on-disk.  That could cause all sorts of fun later.  Yeah, the buffers
should contain only dead tuples ... but, for example, they might not
be hinted dead.  If somebody sets one of those hint bits and then
writes the buffer back out to disk, you've got real problems.

Perhaps there's some gold to be mined by treating truncation locks
differently from other AELs, but I don't think you can just ignore
them on the standby, any more than they can be ignored on the master.

regards, tom lane

Re: [PATCH] Improve geometric types

[Tom Lane]

Emre Hasegeli  writes:
>> BTW how did we end up with the regression differences? Presumably you've
>> tried that on your machine and it passed. So if we adjust the expected
>> file, won't it fail on some other machines?

> I had another patch to check for -0 inside float{4,8}_{div,mul}().  I
> dropped it on the last set of patches, so the tests were broken.  I
> get -0 as a result of -x * 0 both on Mac and Linux.

I'll bet a good deal of money that you'll find that does not hold
true across the whole buildfarm.

regards, tom lane

Re: Pre-v11 appearances of the word "procedure" in v11 docs

[Peter Geoghegan]

On Fri, Aug 17, 2018 at 7:15 AM, Peter Eisentraut
 wrote:
> Attached are my proposed patches.

I take it that you propose all 3 for backpatch to v11?

-- 
Peter Geoghegan

Re: InsertPgAttributeTuple() and attcacheoff

[Peter Eisentraut]

On 14/08/2018 17:52, Robert Haas wrote:
> On Tue, Aug 14, 2018 at 3:50 PM, Tom Lane  wrote:
>> Peter Eisentraut  writes:
>>> It seems to me that it would make sense if InsertPgAttributeTuple() were
>>> to set attcacheoff to -1 instead of taking it from the caller.
>>
>> Looked this over, no objections.
>>
>> I wonder whether we should set that field to -1 when we *read*
>> pg_attribute rows from disk, and be less fussed about what gets written
>> out.  The only real advantage is that this'd protect us from foolish
>> manual changes to pg_attribute.attcacheoff entries, but that doesn't
>> seem negligible.
> 
> I wouldn't object to forcibly writing in -1 when we read the data, but
> I don't think it's a good idea to let values other than -1 get written
> to the disk.  User-visible random nonsense in system catalogs seems
> like too much of a foot-gun to me.

I agree.  Committed as presented then.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Alexander Korotkov]

On Fri, Aug 17, 2018 at 9:55 PM Tom Lane  wrote:
> Alexander Korotkov  writes:
> > On Fri, Aug 17, 2018 at 8:38 PM Tom Lane  wrote:
> >> Alexander Korotkov  writes:
> >>> Yes, that's correct.  On standby read-only queries can tolerate
> >>> concurrent heap truncation.
>
> >> Uh, what???
>
> > VACUUM truncates heap relation only after deletion of all the tuples
> > from tail pages.
>
> Right; the problem I'm worried about is possible accesses to
> already-removed pages by concurrent scans.
>
> > And in md.c we already have logic to return zeroed pages, when trying
> > to read past relation in recovery.
>
> But then you are injecting bad pages into the shared buffer arena.
> In any case, depending on that behavior seems like a bad idea, because
> it's a pretty questionable kluge in itself.
>
> Another point is that the truncation code attempts to remove all
> to-be-truncated-away pages from the shared buffer arena, but that only
> works if nobody else is loading such pages into shared buffers
> concurrently.  In the presence of concurrent scans, we might be left
> with valid-looking buffers for pages that have been truncated away
> on-disk.  That could cause all sorts of fun later.  Yeah, the buffers
> should contain only dead tuples ... but, for example, they might not
> be hinted dead.  If somebody sets one of those hint bits and then
> writes the buffer back out to disk, you've got real problems.
>
> Perhaps there's some gold to be mined by treating truncation locks
> differently from other AELs, but I don't think you can just ignore
> them on the standby, any more than they can be ignored on the master.

Thank you for the explanation.  I see that injecting past OEF pages
into shared buffers doesn't look good.  I start thinking about letting
caller of ReadBuffer() (or its variation) handle past OEF situation.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 08/17/2018 08:56 PM, Tom Lane wrote:
> Emre Hasegeli  writes:
>>> BTW how did we end up with the regression differences? Presumably you've
>>> tried that on your machine and it passed. So if we adjust the expected
>>> file, won't it fail on some other machines?
> 
>> I had another patch to check for -0 inside float{4,8}_{div,mul}().  I
>> dropped it on the last set of patches, so the tests were broken.  I
>> get -0 as a result of -x * 0 both on Mac and Linux.
> 
> I'll bet a good deal of money that you'll find that does not hold
> true across the whole buildfarm.
> 

Hmm, yeah. Based on past experience, the powerpc machines are likely to
stumble on this.

FWIW my understanding is that these failures actually happen in new
tests, it's not an issue introduced by this patch series.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: docs: note ownership requirement for refreshing materialized views

[Jonathan S. Katz]

> On Aug 17, 2018, at 9:21 AM, Tom Lane  wrote:
> 
> Dave Cramer  writes:
>> So it seems this patch is being ignored in this thread.
> 
> Well, Jonathan did kind of hijack what appears to be a thread about
> documentation (with an already-committed fix).

I apologize if it was interpreted as hijacking, I had brought it up in
my initial reply to Dian, as I’ve seen others do similarly on threads.

> I'd suggest reposting that patch in its own thread and adding it to
> the next CF.

I will go ahead and do this.

Thanks, and thank you for the patch Dian.

Jonathan



signature.asc
Description: Message signed with OpenPGP

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Tom Lane]

Alexander Korotkov  writes:
> On Fri, Aug 17, 2018 at 9:55 PM Tom Lane  wrote:
>> Another point is that the truncation code attempts to remove all
>> to-be-truncated-away pages from the shared buffer arena, but that only
>> works if nobody else is loading such pages into shared buffers
>> concurrently.  In the presence of concurrent scans, we might be left
>> with valid-looking buffers for pages that have been truncated away
>> on-disk.  That could cause all sorts of fun later.  Yeah, the buffers
>> should contain only dead tuples ... but, for example, they might not
>> be hinted dead.  If somebody sets one of those hint bits and then
>> writes the buffer back out to disk, you've got real problems.

> Thank you for the explanation.  I see that injecting past OEF pages
> into shared buffers doesn't look good.  I start thinking about letting
> caller of ReadBuffer() (or its variation) handle past OEF situation.

That'd still have the same race condition, though: between the time
we start to drop the doomed pages from shared buffers, and the time
we actually perform ftruncate, concurrent scans could re-load such
pages into shared buffers.

Could it work to ftruncate first and flush shared buffers after?
Probably not, I think the write-back-dirty-hint-bits scenario
breaks that one.

If this were easy, we'd have fixed it years ago :-(.  It'd sure
be awfully nice not to need AEL during autovacuum, even transiently;
but I'm not sure how we get there without adding an unpleasant amount
of substitute locking in table scans.

regards, tom lane

Fix for REFRESH MATERIALIZED VIEW ownership error message

[Jonathan S. Katz]

Hi,I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW as anon-superuser or table owner yields the following message:    test=> REFRESH MATERIALIZED VIEW blah;    ERROR: must be owner of relation blahThe error message should say "...owner of materialized view..."The attached patch corrects this by setting the "relkind" for theREFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the aclcheckreturns the appropriate error message. The updated patch can be tested as such:    CREATE ROLE bar LOGIN;    CREATE TABLE a (x int);    CREATE MATERIALIZED VIEW b AS SELECT * FROM a;    \c - bar    REFRESH MATERIALIZED VIEW b;    ERROR:  must be owner of materialized view bI'm happy to generate the backpatches for it but wanted to receive feedbackfirst.Thanks,Jonathan[1] https://www.postgresql.org/message-id/55498B5B-0155-4B0E-9B97-23167F8CB380%40excoventures.com

0001-treat-refresh-mat-view-as-mat-view.patch
Description: Binary data


signature.asc
Description: Message signed with OpenPGP

Re: [PATCH] Improve geometric types

[Tom Lane]

Tomas Vondra  writes:
> Hmm, yeah. Based on past experience, the powerpc machines are likely to
> stumble on this.

> FWIW my understanding is that these failures actually happen in new
> tests, it's not an issue introduced by this patch series.

Yeah, we've definitely hit such problems before.  The geometric logic
seems particularly prone to it because it's doing more and subtler
float arithmetic than the rest of the system ... but it's not the sole
source of minus-zero issues.

regards, tom lane

Re: Fix for REFRESH MATERIALIZED VIEW ownership error message

[Alvaro Herrera]

On 2018-Aug-17, Jonathan S. Katz wrote:

> Hi,
> 
> I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW as a
> non-superuser or table owner yields the following message:
> 
> test=> REFRESH MATERIALIZED VIEW blah;
> ERROR: must be owner of relation blah
> 
> The error message should say "...owner of materialized view..."
> 
> The attached patch corrects this by setting the "relkind" for the
> REFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the aclcheck
> returns the appropriate error message. The updated patch can be tested as 
> such:
> 
> CREATE ROLE bar LOGIN;
> CREATE TABLE a (x int);
> CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
> \c - bar
> REFRESH MATERIALIZED VIEW b;
> ERROR:  must be owner of materialized view b
> 
> I'm happy to generate the backpatches for it but wanted to receive feedback
> first.

Maybe add your test to some regress/ file?

As it is cosmetic, my inclination would be not to backpatch it.
However, I don't quite see how this patch can possibly fix the problem,
since the new struct member is not used anywhere AFAICT.

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

Getting NOT NULL constraint from pg_attribute

[Wu Ivy]

Hi developers,

I’m currently building a Postgres C extension that fetch data from a Postgres 
table.
Since the table can be large, in order to prevent memory overrun, I use 
SPI_cursor_fetch to fetch chunks of data. The result rows are saved in 
SPITupleTable* SPI_tuptable and attributes are saved in SPI_tuptable->tupdesc. 
In order to process my data, I need to get information of column nullability 
(whether column has NOT NULL constrain). I can get this information by calling:

TupleDesc tupdesc = SPI_tuptable->tupdesc;
bool is_nullable = TupleDescAttr(tupdesc, column_num - 1) -> attnotnull;
However, the result (is_nullable) is always 0, meaning the column does not have 
NOT NULLl constraint, even for columns that do have the NOT NULL constraint.

Any idea of why is it happening? 
Thanks in advance!

Best,
Ivy

Re: Fix for REFRESH MATERIALIZED VIEW ownership error message

[Dave Cramer]

Dave Cramer

da...@postgresintl.com
www.postgresintl.com


On Fri, 17 Aug 2018 at 18:30, Alvaro Herrera 
wrote:

> On 2018-Aug-17, Jonathan S. Katz wrote:
>
> > Hi,
> >
> > I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW
> as a
> > non-superuser or table owner yields the following message:
> >
> > test=> REFRESH MATERIALIZED VIEW blah;
> > ERROR: must be owner of relation blah
> >
> > The error message should say "...owner of materialized view..."
> >
> > The attached patch corrects this by setting the "relkind" for the
> > REFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the
> aclcheck
> > returns the appropriate error message. The updated patch can be tested
> as such:
> >
> > CREATE ROLE bar LOGIN;
> > CREATE TABLE a (x int);
> > CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
> > \c - bar
> > REFRESH MATERIALIZED VIEW b;
> > ERROR:  must be owner of materialized view b
> >
> > I'm happy to generate the backpatches for it but wanted to receive
> feedback
> > first.
>
> Maybe add your test to some regress/ file?
>
+1

>
> As it is cosmetic, my inclination would be not to backpatch it.
> However, I don't quite see how this patch can possibly fix the problem,
> since the new struct member is not used anywhere AFAICT.
>
> The only place this is used is in aclcheck_error
case OBJECT_MATVIEW:
msg = gettext_noop("permission denied for materialized view %s");
break;

Dave

Re: Fix for REFRESH MATERIALIZED VIEW ownership error message

[Alvaro Herrera]

On 2018-Aug-17, Dave Cramer wrote:

> The only place this is used is in aclcheck_error
> case OBJECT_MATVIEW:
> msg = gettext_noop("permission denied for materialized view %s");
> break;

Yes, but do we pass RefreshMatViewStmt->relkind to that routine?  I
don't see that we do.  Maybe I misread the code.

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

Re: Fix for REFRESH MATERIALIZED VIEW ownership error message

[Dave Cramer]

On Fri, 17 Aug 2018 at 19:35, Alvaro Herrera 
wrote:

> On 2018-Aug-17, Dave Cramer wrote:
>
> > The only place this is used is in aclcheck_error
> > case OBJECT_MATVIEW:
> > msg = gettext_noop("permission denied for materialized view %s");
> > break;
>
> Yes, but do we pass RefreshMatViewStmt->relkind to that routine?  I
> don't see that we do.  Maybe I misread the code.
>

Actually the code path that gets executed is:

case OBJECT_MATVIEW:
msg = gettext_noop("must be owner of materialized view %s");
break;

as I have the patch applied and now see:

\c - test
You are now connected to database "test" as user "test".
test=> refresh materialized view b;
ERROR:  must be owner of materialized view b


Dave Cramer

Re: Getting NOT NULL constraint from pg_attribute

[Tom Lane]

Wu Ivy  writes:
> I’m currently building a Postgres C extension that fetch data from a Postgres 
> table.
> Since the table can be large, in order to prevent memory overrun, I use 
> SPI_cursor_fetch to fetch chunks of data. The result rows are saved in 
> SPITupleTable* SPI_tuptable and attributes are saved in 
> SPI_tuptable->tupdesc. 
> In order to process my data, I need to get information of column nullability 
> (whether column has NOT NULL constrain). I can get this information by 
> calling:

>   TupleDesc tupdesc = SPI_tuptable->tupdesc;
>   bool is_nullable = TupleDescAttr(tupdesc, column_num - 1) -> attnotnull;
> However, the result (is_nullable) is always 0, meaning the column does not 
> have NOT NULLl constraint, even for columns that do have the NOT NULL 
> constraint.

The output columns of a SELECT query are never marked nullable, regardless
of what the source data was.

regards, tom lane

Re: Fix help option of contrib/oid2name

[Michael Paquier]

On August 17, 2018 10:53:48 PM GMT+09:00, Tom Lane  wrote:
> Well, we can't remove the -H option, for that reason.  But I think
> we could get away with repurposing -h to also mean "--host", rather
> than "--help" as it is now.  Seems unlikely that any scripts are
> depending on it to mean --help, nor are users likely to expect that
> when none of our other programs treat it that way.

Yeah, I don't see much point in mapping -h to --help. Let's map silently -H to 
--host then. Would you prefer if -H is still documented? Or would it be better 
if it is not documented, mapping silently?
-- 
Michael

Re: Fix help option of contrib/oid2name

[Tom Lane]

Michael Paquier  writes:
> On August 17, 2018 10:53:48 PM GMT+09:00, Tom Lane  wrote:
>> Well, we can't remove the -H option, for that reason.  But I think
>> we could get away with repurposing -h to also mean "--host", rather
>> than "--help" as it is now.  Seems unlikely that any scripts are
>> depending on it to mean --help, nor are users likely to expect that
>> when none of our other programs treat it that way.

> Yeah, I don't see much point in mapping -h to --help. Let's map silently -H 
> to --host then. Would you prefer if -H is still documented? Or would it be 
> better if it is not documented, mapping silently?

I think it probably needs to stay documented, but we could mark it as
deprecated ...

regards, tom lane

Re: Fix help option of contrib/oid2name

[Michael Paquier]

On August 18, 2018 10:52:33 AM GMT+09:00, Tom Lane  wrote:
> I think it probably needs to stay documented, but we could mark it as
> deprecated ...

Okay, no issues with doing so.

-- 
Michael

Re: Conflict handling for COPY FROM

[Karen Huddleston]

Hi Surafel,

Andrew and I began reviewing your patch. It applied cleanly and seems to mostly 
have the functionality you describe. We did have some comments/questions.

1. It sounded like you added the copy_max_error_limit GUC as part of this patch 
to allow users to specify how many errors they want to swallow with this new 
functionality. The GUC didn't seem to be defined and we saw no mention of it in 
the code. My guess is this might be good to address one of the concerns 
mentioned in the initial email thread of this generating too many transaction 
IDs so it would probably be good to have it.
2. I was curious why you only have support for skipping errors on UNIQUE and 
EXCLUSION constraints and not other kinds of constraints? I'm not sure how 
difficult it would be to add support for those, but it seems they could also be 
useful.
3. We think the wording "ON CONFLICT IGNORE" may not be the clearest 
description of what this is doing since it is writing the failed rows to a file 
for a user to process later, but they are not being ignored. We considered 
things like STASH or LOG as alternatives to IGNORE. Andrew may have some other 
suggestions for wording.
4. We also noticed this has no tests and thought it would be good to add some 
to ensure this functionality works how you intend it and continues to work. We 
started running some SQL to validate this, but haven't gotten the chance to put 
it into a clean test yet. We can send you what we have so far, or we are also 
willing to put a little time in to turn it into tests ourselves that we could 
contribute to this patch.

Thanks for writing this patch!
Karen

Re: Fix help option of contrib/oid2name

[Tatsuro Yamada]

On 2018/08/17 12:42, Tatsuro Yamada wrote:

On 2018/08/17 11:47, Michael Paquier wrote:

On Thu, Aug 16, 2018 at 08:57:57PM +0900, Michael Paquier wrote:

I agree on both points.  Any objections if I apply what's proposed here
on HEAD?


I have been looking at this patch.  And while consistency is nice, I
think that if we are cleaning up this stuff we could do a bit more to
be more consistent with the other binary tools:
- Addition of long options for connection options.
- removal of -h, and use it for host value instead of "-H".
- Use "USERNAME" instead of "NAME" in help().

vacuumlo could also be improved a bit...


Yeah, I'll revise the patch based on your suggestions.


I created WIP patch for oid2name and vacuumlo includes followings:

  oid2name and vacuumlo
  - Add long options

  only oid2name
  - Replace -H with -h
  - Replace NAME with USERNAME

I'll revise their documents and create new patch next week, probably. :)

Regards,
Tatsuro Yamada

diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index 63e360c4c5..3501a9c2cc 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -60,8 +60,25 @@ void		sql_exec_dumpalltbspc(PGconn *, struct options *);
 void
 get_opts(int argc, char **argv, struct options *my_opts)
 {
+	static struct option long_options[] = {
+		{"host", required_argument, NULL, 'h'},
+		{"port", required_argument, NULL, 'p'},
+		{"username", required_argument, NULL, 'U'},
+		{"dbname", required_argument, NULL, 'd'},
+		{"tablename", required_argument, NULL, 't'},
+		{"oid", required_argument, NULL, 'o'},
+		{"filenode", no_argument, NULL, 'f'},
+		{"quiet", no_argument, NULL, 'q'},
+		{"system", no_argument, NULL, 'S'},
+		{"extend", no_argument, NULL, 'x'},
+		{"index", no_argument, NULL, 'i'},
+		{"tablespace", no_argument, NULL, 's'},
+		{NULL, 0, NULL, 0}
+	};
+
 	int			c;
 	const char *progname;
+	int			optindex;
 
 	progname = get_progname(argv[0]);
 
@@ -93,10 +110,25 @@ get_opts(int argc, char **argv, struct options *my_opts)
 	}
 
 	/* get opts */
-	while ((c = getopt(argc, argv, "H:p:U:d:t:o:f:qSxish")) != -1)
+	while ((c = getopt_long(argc, argv, "h:p:U:d:t:o:f:qSxis", long_options, &optindex)) != -1)
 	{
 		switch (c)
 		{
+/* host to connect to */
+			case 'h':
+my_opts->hostname = pg_strdup(optarg);
+break;
+
+/* port to connect to on remote host */
+			case 'p':
+my_opts->port = pg_strdup(optarg);
+break;
+
+/* username */
+			case 'U':
+my_opts->username = pg_strdup(optarg);
+break;
+
 /* specify the database */
 			case 'd':
 my_opts->dbname = pg_strdup(optarg);
@@ -122,46 +154,26 @@ get_opts(int argc, char **argv, struct options *my_opts)
 my_opts->quiet = true;
 break;
 
-/* host to connect to */
-			case 'H':
-my_opts->hostname = pg_strdup(optarg);
-break;
-
-/* port to connect to on remote host */
-			case 'p':
-my_opts->port = pg_strdup(optarg);
-break;
-
-/* username */
-			case 'U':
-my_opts->username = pg_strdup(optarg);
-break;
-
 /* display system tables */
 			case 'S':
 my_opts->systables = true;
 break;
 
-/* also display indexes */
-			case 'i':
-my_opts->indexes = true;
-break;
-
 /* display extra columns */
 			case 'x':
 my_opts->extended = true;
 break;
 
+/* also display indexes */
+			case 'i':
+my_opts->indexes = true;
+break;
+
 /* dump tablespaces only */
 			case 's':
 my_opts->tablespaces = true;
 break;
 
-			case 'h':
-help(progname);
-exit(0);
-break;
-
 			default:
 fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 exit(1);
@@ -176,20 +188,21 @@ help(const char *progname)
 		   "Usage:\n"
 		   "  %s [OPTION]...\n"
 		   "\nOptions:\n"
-		   "  -d DBNAME  database to connect to\n"
-		   "  -f FILENODEshow info for table with given file node\n"
-		   "  -H HOSTNAMEdatabase server host or socket directory\n"
-		   "  -i show indexes and sequences too\n"
-		   "  -o OID show info for table with given OID\n"
-		   "  -p PORTdatabase server port number\n"
-		   "  -q quiet (don't show headers)\n"
-		   "  -s show all tablespaces\n"
-		   "  -S show system objects too\n"
-		   "  -t TABLE   show info for named table\n"
-		   "  -U NAMEconnect as specified database user\n"
-		   "  -V, --version  output version information, then exit\n"
-		   "  -x extended (show additional columns)\n"
-		   "  -?, --help show this help, then exit\n"
+		   "  -f, --filenode FILENODEshow info for table with given file node\n"
+		   "  -i, --indexshow indexes and sequences too\n"
+		   "  -o, --oid=OID  show info for table with given OID\n"
+		   "  -q, --quietquiet (don't show headers)\n"
+		   "  -s, --tablespace   show all tablespaces\n"
+		   "  -S, --system 

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Fabien COELHO]

Hello Marina,

Detailed -r report. I understand from the doc that the retry number on 
the detailed per-statement report is to identify at what point errors 
occur? Probably this is more or less always at the same point on a 
given script, so that the most interesting feature is to report the 
number of retries at the script level.


This may depend on various factors.. for example:
[...]
   21.239   5  36  UPDATE xy3 SET y = y + :delta WHERE x 
= :x3;
   21.360   5  39  UPDATE xy2 SET y = y + :delta WHERE x 
= :x2;


Ok, not always the same point, and you confirm that it identifies where 
the error is raised which leads to a retry.


And you can always get the number of retries at the script level from the 
main report (if only one script is used) or from the report for each script 
(if multiple scripts are used).


Ok.

ISTM that "skipped" transactions are NOT "successful" so there are a 
problem with comments. I believe that your formula are probably right, 
it has more to do with what is "success". For cnt decomposition, ISTM 
that "other transactions" are really "directly successful 
transactions".


I agree with you, but I also think that skipped transactions should not be 
considered errors.


I'm ok with having a special category for them in the explanations, which 
is neither success nor error.



So we can write something like this:


All the transactions are divided into several types depending on their 
execution. Firstly, they can be divided into transactions that we started to 
execute, and transactions which were skipped (it was too late to execute 
them). Secondly, running transactions fall into 2 main types: is there any 
command that got a failure during the last execution of the transaction 
script or not? Thus


Here is an attempt at having a more precise and shorter version, not sure 
it is much better than yours, though:


"""
Transactions are counted depending on their execution and outcome. First
a transaction may have started or not: skipped transactions occur under 
--rate and --latency-limit when the client is too late to execute them. 
Secondly, a started transaction may ultimately succeed or fail on some 
error, possibly after some retries when --max-tries is not one. Thus

"""


the number of all transactions =
 skipped (it was too late to execute them)
 cnt (the number of successful transactions) +
 ecnt (the number of failed transactions).

A successful transaction can have several unsuccessful tries before a
successfull run. Thus

cnt (the number of successful transactions) =
 retried (they got a serialization or a deadlock failure(s), but were
  successfully retried from the very beginning) +
 directly successfull transactions (they were successfully completed on
the first try).


These above description is clearer for me.

I'd suggest to put "ANOTHER_SQL_FAILURE" as the last option, otherwise 
"another" does not make sense yet.


Maybe firstly put a general group, and then special cases?...


I understand it more as a catch all default "none of the above" case.

In TState, field "uint32 retries": maybe it would be simpler to count 
"tries", which can be compared directly to max tries set in the option?


If you mean retries in CState - on the one hand, yes, on the other hand, 
statistics always use the number of retries...


Ok.


The automaton skips to FAILURE on every possible error. I'm wondering 
whether it could do so only on SQL errors, because other fails will 
lead to ABORTED anyway? If there is no good reason to skip to FAILURE 
from some errors, I'd suggest to keep the previous behavior. Maybe the 
good reason is to do some counting, but this means that on eg 
metacommand errors now the script would loop over instead of aborting, 
which does not look like a desirable change of behavior.


Even in the case of meta command errors we must prepare for CSTATE_END_TX and 
the execution of the next script: if necessary, clear the conditional stack 
and rollback the current transaction block.


Seems ok.


commandFailed: I'm not thrilled by the added boolean, which is partially
redundant with the second argument.


Do you mean that it is partially redundant with the argument "cmd" and, for 
example, the meta commands errors always do not cause the abortions of the 
client?


Yes. And also I'm not sure we should want this boolean at all.


[...]
If in such cases one command is placed on several lines, ISTM that the code 
is more understandable if curly brackets are used...


Hmmm. Such basic style changes are avoided because they break 
backpatching, so we try to avoid gratuitous changes unless there is a 
strong added value, which does not seem to be the case here.


--
Fabien.

Re: ToDo: show size of partitioned table

[Mathias Brossard]

On Thu, Aug 16, 2018 at 12:46 AM Pavel Stehule 
wrote:

> čt 16. 8. 2018 v 5:52 odesílatel Mathias Brossard 
> napsal:
>
>> I do have a feedback on the implementation. The code tries to support
>> older PostgreSQL server versions when declarative partitions were not
>> supported before version 10 (relkind value of 'p'). Those versions will
>> never return any result from the query being built. So I would suggest an
>> early return from the function. The upside would be that the query building
>> would be simpler. I can make patch implementing that suggestion if you want.
>>
>
> This is question - maybe we can support older partitioning based on only
> inheritance - and the query can be more exact on PostgreSQL 10 and newer.
>
> Please, send any patch. You are welcome.
>

In my very humble opinion, I would restrict the definition of partitions to
declarative partitioning. My justification would be that partitions all use
inheritance, but not all inheritance is a partition (how would you handle
multiple inheritance).

See patch attached that fails (in a way similar to other features) when
connected to servers with version earlier than 10.0.

 Sincerely,
-- Mathias Brossard


psql-dP-10+only.patch
Description: Binary data

Re: TupleTableSlot abstraction

[Andres Freund]

Hi,

On 2018-08-17 12:10:20 +0530, Ashutosh Bapat wrote:
> We need to add LLVM code to fetch tts_flags and
> perform bit operation on it to get or set slow property. I haven't
> found any precedence for LLVM bit operations in postgresql's JIT code.

There are several, look for the infomask accesses in
slot_compiler_deform.

I'll try to do the adaption later today.

Greetings,

Andres Freund

Re: Doc patch: pg_upgrade page and checkpoint location consistency with replicas

[Paul Bonaud]

I shared the pach in plain textin the email body and figured out that
all other patches are submitted as an attachement. Sorry for that, here
is the patch attached to this email.

Thanks!
Paul

On 17/08/18 01:21, Paul Bonaud wrote:
> Hello,
> 
> Please find below a submission of a patch to the PostgreSQL documentation.
> 
> You can also find the patch in this git commit:
> https://gitlab.com/paulrbr/postgresql/commit/024d3870450df6dcdc69bddbe2de46084b73e3a2.diff
> 
> 
> commit 024d3870450df6dcdc69bddbe2de46084b73e3a2
> Author: Paul B 
> Date:   Thu Aug 16 18:25:22 2018 +0200
> 
> doc: Update pg_upgrade page while checking checkpoint locations
> 
> At the end of the previous step 7. you already stopped the primary
>  server. Which led PostgreSQL to issue a CHECKPOINT command.
> Aksi the end of the step 7. also states that "standby servers can
> remain running until a later step".
> 
> However if you keep your standby server running at this point and
> compare the Latest checkpoint location with your
> stopped primary you will never end up with matching values.
> 
> I found it confusing during my pg_upgrade tests as I tried multiple
> times to end up with the same latest checkpoint location value between
> my primary and standby nodes until I realised PostgreSQL was issuing a
> CHECKPOINT during shutdown which would obviously prevent that.
> 
> I reckon some clarification should be added to the documentation for
> that and that is why I propose this patch.
> 
> Please let me know if you want to phrase it differently or if I am
> missing something out.
> 
> Thank you!
> 
> diff --git a/doc/src/sgml/ref/pgupgrade.sgml
> b/doc/src/sgml/ref/pgupgrade.sgml
> index 6dafb404a11..d51146d641d 100644
> --- a/doc/src/sgml/ref/pgupgrade.sgml
> +++ b/doc/src/sgml/ref/pgupgrade.sgml
> @@ -326,7 +326,8 @@ NET STOP postgresql-&majorversion;
>   against the old primary and standby clusters.  Verify that the
>   Latest checkpoint location values match in all
> clusters.
>   (There will be a mismatch if old standby servers were shut down
> - before the old primary.)  Also, change wal_level to
> + before the old primary or if the old standby servers are still
> running.)
> + Also, change wal_level to
>   replica in the
> postgresql.conf file on the
>   new primary cluster.
>  
> 
> 
> ---
> Paul Bonaud
> 
diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index d51146d641d..6dafb404a11 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -326,8 +326,7 @@ NET STOP postgresql-&majorversion;
  against the old primary and standby clusters.  Verify that the
  Latest checkpoint location values match in all clusters.
  (There will be a mismatch if old standby servers were shut down
- before the old primary or if the old standby servers are still running.)
- Also, change wal_level to
+ before the old primary.)  Also, change wal_level to
  replica in the postgresql.conf file on the
  new primary cluster.
 


signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

On 17-08-2018 10:49, Fabien COELHO wrote:

Hello Marina,

Detailed -r report. I understand from the doc that the retry number 
on the detailed per-statement report is to identify at what point 
errors occur? Probably this is more or less always at the same point 
on a given script, so that the most interesting feature is to report 
the number of retries at the script level.


This may depend on various factors.. for example:
[...]
   21.239   5  36  UPDATE xy3 SET y = y + :delta 
WHERE x = :x3;
   21.360   5  39  UPDATE xy2 SET y = y + :delta 
WHERE x = :x2;


Ok, not always the same point, and you confirm that it identifies
where the error is raised which leads to a retry.


Yes, I confirm this. I'll try to write more clearly about this in the 
documentation...



So we can write something like this:


All the transactions are divided into several types depending on their 
execution. Firstly, they can be divided into transactions that we 
started to execute, and transactions which were skipped (it was too 
late to execute them). Secondly, running transactions fall into 2 main 
types: is there any command that got a failure during the last 
execution of the transaction script or not? Thus


Here is an attempt at having a more precise and shorter version, not
sure it is much better than yours, though:

"""
Transactions are counted depending on their execution and outcome. 
First

a transaction may have started or not: skipped transactions occur
under --rate and --latency-limit when the client is too late to
execute them. Secondly, a started transaction may ultimately succeed
or fail on some error, possibly after some retries when --max-tries is
not one. Thus
"""


Thank you!

I'd suggest to put "ANOTHER_SQL_FAILURE" as the last option, 
otherwise "another" does not make sense yet.


Maybe firstly put a general group, and then special cases?...


I understand it more as a catch all default "none of the above" case.


Ok!

commandFailed: I'm not thrilled by the added boolean, which is 
partially

redundant with the second argument.


Do you mean that it is partially redundant with the argument "cmd" 
and, for example, the meta commands errors always do not cause the 
abortions of the client?


Yes. And also I'm not sure we should want this boolean at all.


Perhaps we can use a separate function to print the messages about 
client's abortion, something like this (it is assumed that all abortions 
happen when processing SQL commands):


static void
clientAborted(CState *st, const char *message)
{
pgbench_error(...,
  "client %d aborted in command %d (SQL) of script 
%d; %s\n",
  st->id, st->command, st->use_file, message);
}

Or perhaps we can use a more detailed failure status so for each type of 
failure we always know the command name (argument "cmd") and whether the 
client is aborted. Something like this (but in comparison with the first 
variant ISTM overly complicated):


/*
 * For the failures during script execution.
 */
typedef enum FailureStatus
{
NO_FAILURE = 0,

/*
 * Failures in meta commands. In these cases the failed transaction is
 * terminated.
 */
META_SET_FAILURE,
META_SETSHELL_FAILURE,
META_SHELL_FAILURE,
META_SLEEP_FAILURE,
META_IF_FAILURE,
META_ELIF_FAILURE,

/*
	 * Failures in SQL commands. In cases of serialization/deadlock 
failures a
	 * failed transaction is re-executed from the very beginning if 
possible;

 * otherwise the failed transaction is terminated.
 */
SERIALIZATION_FAILURE,
DEADLOCK_FAILURE,
OTHER_SQL_FAILURE,  /* other failures in SQL 
commands that are not
 * listed by 
themselves above */

/*
 * Failures while processing SQL commands. In this case the client is
 * aborted.
 */
SQL_CONNECTION_FAILURE
} FailureStatus;


[...]
If in such cases one command is placed on several lines, ISTM that the 
code is more understandable if curly brackets are used...


Hmmm. Such basic style changes are avoided because they break
backpatching, so we try to avoid gratuitous changes unless there is a
strong added value, which does not seem to be the case here.


Ok!

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

libpq stricter integer parsing

[Fabien COELHO]

Follow up on a patch and discussion with Tom, currently integer parsing on 
keywords in libpq is quite loose, resulting in trailing garbage being 
ignored and allowing to hide bugs, eg:


  sh> psql "connect_timeout=2,port=5433"

The timeout is set to 2, and the port directive is silently ignored.
However, URL parsing is stricter, eg on "port".

The attached patch checks integer syntax errors and overflows, and report 
errors.


The pros is that it helps detect bugs. The cons is that some people may 
not want to know about these if it works in the end.


--
Fabien.diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 49de975e7f..94d114562a 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1579,6 +1579,16 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 
 

+
+   
+Integer values expected for keywords port,
+connect_timeout,
+keepalives_idle,
+keepalives_interval and
+keepalives_timeout are parsed strictly.
+Versions of libpq before
+PostgreSQL 12 accepted trailing garbage or overflows.
+   
   
  
 
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 9315a27561..15280dc208 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1586,6 +1586,35 @@ useKeepalives(PGconn *conn)
 	return val != 0 ? 1 : 0;
 }
 
+/*
+ * Parse integer, report any syntax error, and tell if all is well.
+ */
+static bool
+strict_atoi(const char *str, int *val, PGconn *conn, const char *context)
+{
+	char 	*endptr;
+
+	errno = 0;
+	*val = strtol(str, &endptr, 10);
+
+	if (errno != 0)
+	{
+		appendPQExpBuffer(&conn->errorMessage,
+		  libpq_gettext("invalid integer \"%s\" for keyword \"%s\": %s\n"),
+		  str, context, strerror(errno));
+		return false;
+	}
+	else if (*endptr != '\0')
+	{
+		appendPQExpBuffer(&conn->errorMessage,
+		  libpq_gettext("invalid integer \"%s\" for keyword \"%s\"\n"),
+		  str, context);
+		return false;
+	}
+
+	return true;
+}
+
 #ifndef WIN32
 /*
  * Set the keepalive idle timer.
@@ -1598,7 +1627,9 @@ setKeepalivesIdle(PGconn *conn)
 	if (conn->keepalives_idle == NULL)
 		return 1;
 
-	idle = atoi(conn->keepalives_idle);
+	if (!strict_atoi(conn->keepalives_idle, &idle, conn, "keepalives_idle"))
+		return 0;
+
 	if (idle < 0)
 		idle = 0;
 
@@ -1630,7 +1661,9 @@ setKeepalivesInterval(PGconn *conn)
 	if (conn->keepalives_interval == NULL)
 		return 1;
 
-	interval = atoi(conn->keepalives_interval);
+	if (!strict_atoi(conn->keepalives_interval, &interval, conn, "keepalives_interval"))
+		return 0;
+
 	if (interval < 0)
 		interval = 0;
 
@@ -1663,7 +1696,9 @@ setKeepalivesCount(PGconn *conn)
 	if (conn->keepalives_count == NULL)
 		return 1;
 
-	count = atoi(conn->keepalives_count);
+	if (!strict_atoi(conn->keepalives_count, &count, conn, "keepalives_count"))
+		return 0;
+
 	if (count < 0)
 		count = 0;
 
@@ -1697,13 +1732,17 @@ setKeepalivesWin32(PGconn *conn)
 	int			idle = 0;
 	int			interval = 0;
 
-	if (conn->keepalives_idle)
-		idle = atoi(conn->keepalives_idle);
+	if (conn->keepalives_idle &&
+		!strict_atoi(conn->keepalives_idle, &idle, conn, "keepalives_idle"))
+		return 0;
+
+	if (conn->keepalives_interval &&
+		!strict_atoi(conn->keepalives_interval, &interval, conn, "keepalives_interval"))
+		return 0;
+
 	if (idle <= 0)
 		idle = 2 * 60 * 60;		/* 2 hours = default */
 
-	if (conn->keepalives_interval)
-		interval = atoi(conn->keepalives_interval);
 	if (interval <= 0)
 		interval = 1;			/* 1 second = default */
 
@@ -1784,7 +1823,9 @@ connectDBStart(PGconn *conn)
 			thisport = DEF_PGPORT;
 		else
 		{
-			thisport = atoi(ch->port);
+			if (!strict_atoi(ch->port, &thisport, conn, "port"))
+goto connect_errReturn;
+
 			if (thisport < 1 || thisport > 65535)
 			{
 appendPQExpBuffer(&conn->errorMessage,
@@ -1916,7 +1957,9 @@ connectDBComplete(PGconn *conn)
 	 */
 	if (conn->connect_timeout != NULL)
 	{
-		timeout = atoi(conn->connect_timeout);
+		if (!strict_atoi(conn->connect_timeout, &timeout, conn, "connect_timeout"))
+			return 0;
+
 		if (timeout > 0)
 		{
 			/*
@@ -1927,6 +1970,8 @@ connectDBComplete(PGconn *conn)
 			if (timeout < 2)
 timeout = 2;
 		}
+		else /* negative means 0 */
+			timeout = 0;
 	}
 
 	for (;;)

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Fabien COELHO]

commandFailed: I'm not thrilled by the added boolean, which is partially
redundant with the second argument.


Do you mean that it is partially redundant with the argument "cmd" and, 
for example, the meta commands errors always do not cause the abortions of 
the client?


Yes. And also I'm not sure we should want this boolean at all.


Perhaps we can use a separate function to print the messages about client's 
abortion, something like this (it is assumed that all abortions happen when 
processing SQL commands):


static void
clientAborted(CState *st, const char *message)


Possibly.

Or perhaps we can use a more detailed failure status so for each type of 
failure we always know the command name (argument "cmd") and whether the 
client is aborted. Something like this (but in comparison with the first 
variant ISTM overly complicated):


I agree., I do not think that it would be useful given that the same thing 
is done on all meta-command error cases in the end.


--
Fabien.

Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

[Marina Polyakova]

On 17-08-2018 14:04, Fabien COELHO wrote:

...
Or perhaps we can use a more detailed failure status so for each type 
of failure we always know the command name (argument "cmd") and 
whether the client is aborted. Something like this (but in comparison 
with the first variant ISTM overly complicated):


I agree., I do not think that it would be useful given that the same
thing is done on all meta-command error cases in the end.


Ok!

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: docs: note ownership requirement for refreshing materialized views

[Dave Cramer]

Dave Cramer


On Thu, 16 Aug 2018 at 18:27, Jonathan S. Katz <
jonathan.k...@excoventures.com> wrote:

>
> On Aug 16, 2018, at 1:05 AM, Jonathan S. Katz <
> jonathan.k...@excoventures.com> wrote:
>
>
> On Aug 15, 2018, at 9:15 PM, Michael Paquier  wrote:
>
> On Wed, Aug 15, 2018 at 09:06:34PM -0400, Jonathan S. Katz wrote:
>
> I played around with this feature a bit and did see this was the case.
> Also while playing around I noticed the error message was as such:
>
> test=> REFRESH MATERIALIZED VIEW blah;
> ERROR: must be owner of relation blah
>
> But it’s not a relation, it’s a materialized view. I attached a patch
> that I think should fix this. Kudos to Dave Cramer who was
> sitting next to me helping me to locate files and confirm assumptions.
>
>
> A relation may be a materialized view, no?  The ACL check happens in
> RangeVarCallbackOwnsTable by the way (look at ExecRefreshMatView in
> matview.c).
>
>
> Comment on the RangeVarCallbackOwnsTable func (abbr):
>
>/*
> * This is intended as a callback for RangeVarGetRelidExtended().  It
> allows
> * the relation to be locked only if (1) it's a plain table,
> materialized
> * view, or TOAST table and (2) the current user is the owner (or the
> * superuser).  This meets the permission-checking needs of CLUSTER,
> REINDEX
> * TABLE, and REFRESH MATERIALIZED VIEW; we expose it here so that it
> can be
> * used by all.
> */
>
> So it’s sharing the permission checking needs amongst all of those
> commands.
>
> As a user I could be confused if I saw the above error message, esp.
> because
> the behavior of REFRESH .. is specific to materialized views.
>
>
> With encouragement from Dave, let me demonstrate what the proposed patch
> does to fix the behavior. The steps, running from my “jkatz” user:
>
> CREATE ROLE bar LOGIN;
> CREATE TABLE a (x int);
> CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
> \c - bar
> REFRESH MATERIALIZED VIEW b;
> ERROR:  must be owner of materialized view b
>
> vs. the existing error message which I posted further upthread.
>
> Thanks,
>
> Jonathan
>

So it seems this patch is being ignored in this thread.
AFAICT, in aclcheck_error() the only way to get to the following sub case
in the case ACLCHECK_NOT_OWNER

case OBJECT_MATVIEW:
msg = gettext_noop("must be owner of materialized view %s");
break;

is if the patch is applied ?

Regards,

Dave

Re: docs: note ownership requirement for refreshing materialized views

[Dian Fay]

Jonathan's patch seems like a good idea to me from a user POV, but then 
I just showed up the other day so I don't really have anything of 
substance to add.


On 8/17/18 9:08 AM, Dave Cramer wrote:


Dave Cramer


On Thu, 16 Aug 2018 at 18:27, Jonathan S. Katz 
> wrote:




On Aug 16, 2018, at 1:05 AM, Jonathan S. Katz
mailto:jonathan.k...@excoventures.com>> wrote:



On Aug 15, 2018, at 9:15 PM, Michael Paquier
mailto:mich...@paquier.xyz>> wrote:

On Wed, Aug 15, 2018 at 09:06:34PM -0400, Jonathan S. Katz wrote:

I played around with this feature a bit and did see this was
the case.
Also while playing around I noticed the error message was as such:

test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blah

But it’s not a relation, it’s a materialized view. I attached a
patch
that I think should fix this. Kudos to Dave Cramer who was
sitting next to me helping me to locate files and confirm
assumptions.


A relation may be a materialized view, no?  The ACL check happens in
RangeVarCallbackOwnsTable by the way (look at ExecRefreshMatView in
matview.c).


Comment on the RangeVarCallbackOwnsTable func (abbr):

   /*
* This is intended as a callback for
RangeVarGetRelidExtended().  It allows
* the relation to be locked only if (1) it's a plain table,
materialized
* view, or TOAST table and (2) the current user is the owner
(or the
* superuser).  This meets the permission-checking needs of
CLUSTER, REINDEX
* TABLE, and REFRESH MATERIALIZED VIEW; we expose it here so
that it can be
* used by all.
*/

So it’s sharing the permission checking needs amongst all of
those commands.

As a user I could be confused if I saw the above error message,
esp. because
the behavior of REFRESH .. is specific to materialized views.


With encouragement from Dave, let me demonstrate what the proposed
patch
does to fix the behavior. The steps, running from my “jkatz” user:

CREATE ROLE bar LOGIN;
CREATE TABLE a (x int);
CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
\c - bar
REFRESH MATERIALIZED VIEW b;
ERROR:  must be owner of materialized view b

vs. the existing error message which I posted further upthread.

Thanks,

Jonathan


So it seems this patch is being ignored in this thread.
AFAICT, in aclcheck_error() the only way to get to the following sub 
case in the case ACLCHECK_NOT_OWNER


case OBJECT_MATVIEW:
msg = gettext_noop("must be owner of materialized view %s");
break;

is if the patch is applied ?

Regards,

Dave

Re: Fix help option of contrib/oid2name

[Alvaro Herrera]

On 2018-Aug-17, Tatsuro Yamada wrote:

>   only oid2name
>   - Replace -H with -h

I think this one is a bad idea, as it'll break scripts.

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

Re: docs: note ownership requirement for refreshing materialized views

[Tom Lane]

Dave Cramer  writes:
> So it seems this patch is being ignored in this thread.

Well, Jonathan did kind of hijack what appears to be a thread about
documentation (with an already-committed fix).

I'd suggest reposting that patch in its own thread and adding it to
the next CF.

regards, tom lane

Re: Facility for detecting insecure object naming

[Bruce Momjian]

On Thu, Aug 16, 2018 at 10:58:21PM -0400, Chapman Flack wrote:
> On 08/16/18 21:31, Bruce Momjian wrote:
> 
> > I understand you don't like that a search_path changed by a function is
> > passed down to functions it calls, but what would you like the system to
> > use as a search path for called functions?  The search path of the
> > session?
> 
> This is beginning to sound like an exercise in /emulating/ lexical scoping
> in a system that can run bits of code supplied in many different PLs, not
> all of which can necessarily participate in "real" lexical scoping.
> 
> It could look like another flavor of setting a GUC. There's SET SESSION,
> SET LOCAL, and maybe now SET LEXICAL. The key to the emulation is to
> temporarily unstack the SET LEXICAL when the function it "lexically"
> applies to makes calls to other functions. And restack it when those
> other functions return.

Right.  My point is that people don't like dynamic scoping in Perl
because it overrides the lexical scoping implicit in the source code
structure.  For Postgres objects, like functions, there is no implicit
scope --- each object is defined and managed on its own.  So, the
problem isn't that dynamic scoping is overriding implicit scoping, but
that we only have dynamic scoping, and there is no implicit scoping to
override.

So, to move this forward, we have to have some way of defining implicit
scoping.  It could be:

o  other objects in the same schema
o  owned by the same user
o  a member of the same extension
o  a SET search_path in the function
o  the search_path at object creation time
o  objects referenced at object creation time

(I guess Oracle's package syntax helps them here.)  You have to define
some implicit scoping so you don't have to rely on dynamic scoping.

Two of Robert's problem cases were expression indexes that are later
reindexed, and calling an extension that wants to call functions from the
same extension, even if the search_path does not include the extension's
schema.  So, which implicit scoping rules would fix those problems?

In addition, you have people adding, updating, and removing objects
while these functions are required to still run properly.  That is
something that also doesn't typically happen in a Perl program, because
the source files or package APIs are assumed to be tightly managed.

Also, with Perl, you have a layer of objects, from built-in ones, to
packages, to application-level objects.  With Postgres, they are all
thrown together into the same tables, and there is no filtering of
scopes.

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

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

Re: Fix help option of contrib/oid2name

[Tom Lane]

Alvaro Herrera  writes:
> On 2018-Aug-17, Tatsuro Yamada wrote:
>> only oid2name
>> - Replace -H with -h

> I think this one is a bad idea, as it'll break scripts.

Well, we can't remove the -H option, for that reason.  But I think
we could get away with repurposing -h to also mean "--host", rather
than "--help" as it is now.  Seems unlikely that any scripts are
depending on it to mean --help, nor are users likely to expect that
when none of our other programs treat it that way.

regards, tom lane

Re: Pre-v11 appearances of the word "procedure" in v11 docs

[Peter Eisentraut]

Attached are my proposed patches.  The first is the documentation
change, which basically just substitutes the words, with some occasional
rephrasing.  And then patches to extend the syntaxes of CREATE OPERATOR,
CREATE TRIGGER, and CREATE EVENT TRIGGER to accept FUNCTION in place of
PROCEDURE.  I decided to do that because with the adjustments from the
first patch, the documentation had become comically inconsistent in some
places and it was easier to just fix the underlying problem than to
explain the reasons for the inconsistencies everywhere.  I didn't go
around change all the commands in contrib modules etc. to keep the patch
size under control.  This could perhaps be done later.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 142d34b7532fdd8e439bdf21b00983c13f4325a5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 15 Aug 2018 17:01:39 +0200
Subject: [PATCH 1/3] doc: Update uses of the word "procedure"

Historically, the term procedure was used as a synonym for function in
Postgres/PostgreSQL.  Now we have procedures as separate objects from
functions, so we need to clean up the documentation to not mix those
terms.

In particular, mentions of "trigger procedures" are changed to "trigger
functions", and access method "support procedures" are changed to
"support functions".  (The latter already used FUNCTION in the SQL
syntax anyway.)  Also, the terminology in the SPI chapter has been
cleaned up.

A few tests, examples, and code comments are also adjusted to be
consistent with documentation changes, but not everything.
---
 doc/src/sgml/brin.sgml| 52 ++--
 doc/src/sgml/catalogs.sgml| 20 ++---
 doc/src/sgml/event-trigger.sgml   |  6 +-
 doc/src/sgml/func.sgml|  2 +-
 doc/src/sgml/plhandler.sgml   |  2 +-
 doc/src/sgml/plperl.sgml  |  2 +-
 doc/src/sgml/plpgsql.sgml | 34 
 doc/src/sgml/pltcl.sgml   | 40 -
 doc/src/sgml/ref/alter_opfamily.sgml  |  4 +-
 doc/src/sgml/ref/create_language.sgml |  2 +-
 doc/src/sgml/ref/create_opclass.sgml  |  6 +-
 doc/src/sgml/ref/create_operator.sgml |  8 +-
 doc/src/sgml/ref/create_trigger.sgml  |  2 +-
 doc/src/sgml/spi.sgml | 83 +--
 doc/src/sgml/xindex.sgml  |  2 +-
 doc/src/sgml/xplang.sgml  |  4 +-
 src/backend/access/gin/ginvalidate.c  |  2 +-
 src/backend/access/gist/gistvalidate.c|  2 +-
 src/backend/access/hash/hashutil.c|  4 +-
 src/backend/access/hash/hashvalidate.c|  4 +-
 src/backend/access/spgist/spgvalidate.c   |  2 +-
 src/backend/commands/opclasscmds.c| 30 +++
 src/bin/psql/describe.c   |  2 +-
 src/include/access/hash.h | 12 +--
 src/test/regress/expected/alter_generic.out   | 16 ++--
 src/test/regress/expected/create_operator.out |  6 +-
 src/test/regress/sql/create_operator.sql  |  6 +-
 27 files changed, 174 insertions(+), 181 deletions(-)

diff --git a/doc/src/sgml/brin.sgml b/doc/src/sgml/brin.sgml
index f02e061bc1..f47e1968a4 100644
--- a/doc/src/sgml/brin.sgml
+++ b/doc/src/sgml/brin.sgml
@@ -537,7 +537,7 @@ Extensibility
 } BrinOpcInfo;
 
   
BrinOpcInfo.oi_opaque can 
be used by the
-  operator class routines to pass information between support procedures
+  operator class routines to pass information between support functions
   during an index scan.
  
 
@@ -587,27 +587,27 @@ Extensibility
   defined by the user for other data types using equivalent definitions,
   without having to write any source code; appropriate catalog entries being
   declared is enough.  Note that assumptions about the semantics of operator
-  strategies are embedded in the support procedures' source code.
+  strategies are embedded in the support functions' source code.
  
 
  
   Operator classes that implement completely different semantics are also
-  possible, provided implementations of the four main support procedures
+  possible, provided implementations of the four main support functions
   described above are written.  Note that backwards compatibility across major
-  releases is not guaranteed: for example, additional support procedures might
+  releases is not guaranteed: for example, additional support functions might
   be required in later releases.
  
 
  
   To write an operator class for a data type that implements a totally
-  ordered set, it is possible to use the minmax support procedures
+  ordered set, it is possible to use the minmax support functions
   alongside the corresponding operators, as shown in
   .
-  All operator class members (procedures and operators) are mandatory.
+  All operator class members (functions and operato

Re: Memory leak with CALL to Procedure with COMMIT.

[Peter Eisentraut]

On 16/08/2018 19:26, Tom Lane wrote:
>> When a CALL has output parameters, the portal uses the strategy
>> PORTAL_UTIL_SELECT instead of PORTAL_MULTI_QUERY.  Using
>> PORTAL_UTIL_SELECT causes the portal's snapshot to be registered with
>> the current resource owner (portal->holdSnapshot).  I'm not sure why
>> this is done for one kind of portal strategy but not the other.

> I'm a bit confused by that statement.  AFAICS, for both PortalRunUtility
> and PortalRunMulti, setHoldSnapshot is only passed as true by
> FillPortalStore, so registering the snapshot should happen (or not) the
> same way for either portal execution strategy.  What scenarios are you
> comparing here, exactly?

The call to PortalRunMulti() in FillPortalStore() is for
PORTAL_ONE_RETURNING and PORTAL_ONE_MOD_WITH, not for
PORTAL_MULTI_QUERY.  For PORTAL_MULTI_QUERY, FillPortalStore() isn't
called; PortalRun() calls PortalRunMulti() directly with setHoldSnapshot
= false.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH] Improve geometric types

[Tomas Vondra]

Hi,

the buildfarm seems to be mostly happy so far, so I've taken a quick
look at the remaining two parts. The patches still apply, but I'm
getting plenty of failures in regression tests, due to 0.0 being
replaced by -0.0.

This reminds me 74294c7301, except that these patches don't seem to
remove any such checks by mistake. Instead it seems to be caused by
simply switching to float8_ methods. The attached patch fixes the issue
for me, although I'm not claiming it's the right way to fix it.

Another thing I noticed is the last few lines from line_interpt_line are
actually unreachable, because there's now 'else return false' branch.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index e0a9a0fa4f..97b3349ff8 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -1251,6 +1251,14 @@ line_interpt_line(Point *result, LINE *l1, LINE *l2)
 	   float8_mi(float8_mul(l1->A, l2->B),
  float8_mul(l2->A, l1->B)));
 		y = float8_div(-float8_pl(float8_mul(l1->A, x), l1->C), l1->B);
+
+		/* on some platforms, the preceding expression tends to produce -0 */
+		if (x == 0.0)
+			x = 0.0;
+
+		/* on some platforms, the preceding expression tends to produce -0 */
+		if (y == 0.0)
+			y = 0.0;
 	}
 	else if (!FPzero(l2->B))
 	{
@@ -1262,6 +1270,14 @@ line_interpt_line(Point *result, LINE *l1, LINE *l2)
 	   float8_mi(float8_mul(l2->A, l1->B),
  float8_mul(l1->A, l2->B)));
 		y = float8_div(-float8_pl(float8_mul(l2->A, x), l2->C), l2->B);
+
+		/* on some platforms, the preceding expression tends to produce -0 */
+		if (x == 0.0)
+			x = 0.0;
+
+		/* on some platforms, the preceding expression tends to produce -0 */
+		if (y == 0.0)
+			y = 0.0;
 	}
 	else
 		return false;
@@ -1798,6 +1814,12 @@ point_send(PG_FUNCTION_ARGS)
 static inline void
 point_construct(Point *result, float8 x, float8 y)
 {
+	if (x == 0.0)
+		x = 0.0;
+
+	if (y == 0.0)
+		y = 0.0;
+
 	result->x = x;
 	result->y = y;
 }

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Alexander Korotkov]

On Thu, Aug 16, 2018 at 2:16 PM Alexander Korotkov
 wrote:
> On Tue, Aug 14, 2018 at 12:05 PM Masahiko Sawada  
> wrote:
> >
> > On Wed, Feb 28, 2018 at 11:24 PM, Ivan Kartyshov
> >  wrote:
> > > The main goal of my changes is to let long read-only transactions run on
> > > replica if hot_standby_feedback is turned on.
> >
> > FWIW the idea proposed on the thread[1], which allows us to disable
> > the heap truncation by vacuum per tables, might help this issue. Since
> > the original problem on that thread is a performance problem an
> > alternative proposal would be better, but I think the way would be
> > helpful for this issue too and is simple. A downside of that idea is
> > that there is additional work for user to configure the reloption to
> > each tables.
>
> Thank you for pointing this thread.  It's possible to cope this
> downside to introduce GUC which would serve as default, when reloption
> is not defined.  But real downside is inability to truncate the heap.
> So, options are good if they provide workaround for users, but that
> shouldn't stop us from fixing issues around heap truncation.

So, do we have any objections to committing this?

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Andres Freund]

Hi,

On 2018-08-17 18:00:20 +0300, Alexander Korotkov wrote:
> So, do we have any objections to committing this?

I think this needs more review by other senior hackers in the community.

Greetings,

Andres Freund

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Tom Lane]

Andres Freund  writes:
> On 2018-08-17 18:00:20 +0300, Alexander Korotkov wrote:
>> So, do we have any objections to committing this?

> I think this needs more review by other senior hackers in the community.

TBH it sounds like a horrible hack.  Disable vacuum truncation?
That can't be a good idea.  If it were something we were doing
behind the scenes as a temporary expedient, maybe we could hold
our noses and do it for awhile.  But once you introduce a reloption
based on this, you can't ever get rid of it.

I think we need to spend more effort looking for a less invasive,
less compromised solution.

regards, tom lane

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Andres Freund]

On 2018-08-17 11:35:40 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2018-08-17 18:00:20 +0300, Alexander Korotkov wrote:
> >> So, do we have any objections to committing this?
> 
> > I think this needs more review by other senior hackers in the community.
> 
> TBH it sounds like a horrible hack.  Disable vacuum truncation?

There's another patch, which I thought Alexander was referring to, that
does something a bit smarger.  On a super short skim it seems to
introduce a separate type of AEL lock that's not replicated, by my
reading?

Greetings,

Andres Freund

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Alexander Korotkov]

On Fri, Aug 17, 2018 at 6:41 PM Andres Freund  wrote:
> On 2018-08-17 11:35:40 -0400, Tom Lane wrote:
> > Andres Freund  writes:
> > > On 2018-08-17 18:00:20 +0300, Alexander Korotkov wrote:
> > >> So, do we have any objections to committing this?
> >
> > > I think this needs more review by other senior hackers in the community.
> >
> > TBH it sounds like a horrible hack.  Disable vacuum truncation?
>
> There's another patch, which I thought Alexander was referring to, that
> does something a bit smarger.  On a super short skim it seems to
> introduce a separate type of AEL lock that's not replicated, by my
> reading?

Yes, that's correct.  On standby read-only queries can tolerate
concurrent heap truncation.  So, there is no point to replicate AEL to
standby.  Previous versions of patch tries to do that by introducing
some flag.  But it appears that correct way to do this is just new
non-replicated type of AEL lock.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Slotification of partition tuple conversion

[Amit Khandekar]

Hi,

In [1] , it was shown that the partition tuples are needlessly formed
and deformed during tuple conversion (do_convert_tuple), when the same
operation can be done using tuple slots. This is because the input
slot might already have a deformed tuple.

Attached is a patch tup_convert.patch that does the conversion
directly using input and output tuple slots. This patch is to be
applied on an essential patch posted by Amit Langote in [1] that
arranges for dedicated per-partition slots rather than changing
tupdesc of a single slot. I have rebased that patch from [1] and
attached it here (
Allocate-dedicated-slots-of-partition-tuple_amitlan_rebased.patch).

In tup_convert.patch, wherever ConvertPartitionTupleSlot() and
do_convert_tuple() are used to convert partition tuples, I have
replaced them by a new function ConvertTupleSlot().

ConvertPartitionTupleSlot() is exclusively for conversion of
partition-to-parent and vice versa, so I got rid of this.
do_convert_tuple() seems to be used for tuples belonging to
non-partition tables as well, so I have left such calls untouched. May
be we can "slotify" these tuple conversions later as a separate task.

[1] 
https://www.postgresql.org/message-id/e4f9d743-cd4b-efb0-7574-da21d86a7f36%40lab.ntt.co.jp


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company


tup_convert.patch
Description: Binary data


Allocate-dedicated-slots-of-partition-tuple_amitlan_rebased.patch
Description: Binary data

Re: partitioning - changing a slot's descriptor is expensive

[Amit Khandekar]

On 29 June 2018 at 11:53, Amit Langote  wrote:
> Other issues that you mentioned, such as needless heap_tuple_deform/form
> being invoked, seem less localized (to me) than this particular issue, so
> I created a patch for just this, which is attached with this email.  I'm
> thinking about how to fix the other issues, but will need a bit more time.

I have started a new thread on these tuple forming/deforming issues
[1], and posted there a patch that is to be applied over your
Allocate-dedicated-slots-of-partition-tuple.patch (which I rebased and
posted in that thread) :

[1] 
https://www.postgresql.org/message-id/CAJ3gD9fR0wRNeAE8VqffNTyONS_UfFPRpqxhnD9Q42vZB%2BJvpg%40mail.gmail.com


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Re: [PATCH] Improve geometric types

[Emre Hasegeli]

> the buildfarm seems to be mostly happy so far, so I've taken a quick
> look at the remaining two parts. The patches still apply, but I'm
> getting plenty of failures in regression tests, due to 0.0 being
> replaced by -0.0.

I think we are better off fixing them locally at the moment like your
patch does.  We should consider to eliminate -0 globally for all
floating point based datatypes later.  I simplified and incorporated
your change to line_interpt_line() into mine.

I am not sure about normalising -0s on point_construct().  We
currently allow points to be initialized with -0s.  I think it is fair
for us to return -0 when -x and 0 are multiplied.  That is the current
behavior and the behavior of the float datatypes.  I adjusted the
results of the new regression tests accordingly.

> Another thing I noticed is the last few lines from line_interpt_line are
> actually unreachable, because there's now 'else return false' branch.

Which lines do you mean exactly?  I don't see any being unreachable.


0001-line-fixes-v14.patch
Description: Binary data


0002-geo-tests-v10.patch.gz
Description: GNU Zip compressed data

Re: Index Skip Scan

[Jesper Pedersen]

Hi Peter,

On 08/16/2018 03:48 PM, Peter Geoghegan wrote:

On Wed, Aug 15, 2018 at 11:22 PM, Thomas Munro
 wrote:

* groups and certain aggregates (MIN() and MAX() of suffix index
columns within each group)
* index scans where the scan key doesn't include the leading columns
(but you expect there to be sufficiently few values)
* merge joins (possibly the trickiest and maybe out of range)


FWIW, I suspect that we're going to have the biggest problems in the
optimizer. It's not as if ndistinct is in any way reliable. That may
matter more on average than it has with other path types.



Thanks for sharing this; it is very useful to know.

Best regards,
 Jesper

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 08/17/2018 06:40 PM, Emre Hasegeli wrote:
>> the buildfarm seems to be mostly happy so far, so I've taken a quick
>> look at the remaining two parts. The patches still apply, but I'm
>> getting plenty of failures in regression tests, due to 0.0 being
>> replaced by -0.0.
> 
> I think we are better off fixing them locally at the moment like your
> patch does.  We should consider to eliminate -0 globally for all
> floating point based datatypes later.  I simplified and incorporated
> your change to line_interpt_line() into mine.
> 
> I am not sure about normalising -0s on point_construct().  We
> currently allow points to be initialized with -0s.  I think it is fair
> for us to return -0 when -x and 0 are multiplied.  That is the current
> behavior and the behavior of the float datatypes.  I adjusted the
> results of the new regression tests accordingly.
> 

Hmmm, I need to think about that a bit more.

BTW how did we end up with the regression differences? Presumably you've
tried that on your machine and it passed. So if we adjust the expected
file, won't it fail on some other machines?

>> Another thing I noticed is the last few lines from line_interpt_line are
>> actually unreachable, because there's now 'else return false' branch.
> 
> Which lines do you mean exactly?  I don't see any being unreachable.
> 

Apologies, I got confused - there are no unreachable lines.


-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Tom Lane]

Alexander Korotkov  writes:
> On Fri, Aug 17, 2018 at 6:41 PM Andres Freund  wrote:
>> There's another patch, which I thought Alexander was referring to, that
>> does something a bit smarger.  On a super short skim it seems to
>> introduce a separate type of AEL lock that's not replicated, by my
>> reading?

> Yes, that's correct.  On standby read-only queries can tolerate
> concurrent heap truncation.

Uh, what???

regards, tom lane

Re: Index Skip Scan

[Peter Geoghegan]

On Thu, Aug 16, 2018 at 4:10 PM, Thomas Munro
 wrote:
> Can you give an example of problematic ndistinct underestimation?

Yes. See 
https://postgr.es/m/cakuk5j12qokfh88tqz-ojmsibg2qyjm7k7hlnbyi3ze+y5b...@mail.gmail.com,
for example. That's a complaint about an underestimation specifically.

This seems to come up about once every 3 years, at least from my
perspective. I'm always surprised that ndistinct doesn't get
implicated in bad query plans more frequently.

> I suppose you might be able to defend against that in the executor: if
> you find that you've done an unexpectedly high number of skips, you
> could fall back to regular next-tuple mode.  Unfortunately that's
> require the parent plan node to tolerate non-unique results.

I like the idea of dynamic fallback in certain situations, but the
details always seem complicated.

-- 
Peter Geoghegan

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Alexander Korotkov]

On Fri, Aug 17, 2018 at 8:38 PM Tom Lane  wrote:
> Alexander Korotkov  writes:
> > On Fri, Aug 17, 2018 at 6:41 PM Andres Freund  wrote:
> >> There's another patch, which I thought Alexander was referring to, that
> >> does something a bit smarger.  On a super short skim it seems to
> >> introduce a separate type of AEL lock that's not replicated, by my
> >> reading?
>
> > Yes, that's correct.  On standby read-only queries can tolerate
> > concurrent heap truncation.
>
> Uh, what???

VACUUM truncates heap relation only after deletion of all the tuples
from tail pages.  So, on standby heap truncation record would be
replayed only after heap tuples deletion records are replayed.
Therefore, if query on standby should see some of those tuples, WAL
replay stop (or query cancel) should happened before corresponding
tuples being deleted by our recovery conflict with snapshot logic.
When we're going to replay heap truncation record, no query should see
records in the tail pages.

And in md.c we already have logic to return zeroed pages, when trying
to read past relation in recovery.

/*
 * Short read: we are at or past EOF, or we read a partial block at
 * EOF.  Normally this is an error; upper levels should never try to
 * read a nonexistent block.  However, if zero_damaged_pages is ON or
 * we are InRecovery, we should instead return zeroes without
 * complaining.  This allows, for example, the case of trying to
 * update a block that was later truncated away.
 */
if (zero_damaged_pages || InRecovery)
MemSet(buffer, 0, BLCKSZ);
else
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
 errmsg("could not read block %u in file \"%s\":
read only %d of %d bytes",
blocknum, FilePathName(v->mdfd_vfd),
nbytes, BLCKSZ)));

But, I'm concerned if FileSeek() could return error.  And also what
_mdfd_getseg() would do on truncated segment.  It seems that in
recovery, it will automatically extend the relation.  That
unacceptable for this purpose.  So, sorry for bothering, this patch
definitely needs to be revised.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [PATCH] Improve geometric types

[Emre Hasegeli]

> BTW how did we end up with the regression differences? Presumably you've
> tried that on your machine and it passed. So if we adjust the expected
> file, won't it fail on some other machines?

I had another patch to check for -0 inside float{4,8}_{div,mul}().  I
dropped it on the last set of patches, so the tests were broken.  I
get -0 as a result of -x * 0 both on Mac and Linux.

Performance improvements for src/port/snprintf.c

[Tom Lane]

Over in the what-about-%m thread, we speculated about replacing the
platform's *printf functions if they didn't support %m, which would
basically mean using src/port/snprintf.c on all non-glibc platforms,
rather than only on Windows as happens right now (ignoring some
obsolete platforms with busted snprintf's).

I've been looking into the possible performance consequences of that,
in particular comparing snprintf.c to the library versions on macOS,
FreeBSD, OpenBSD, and NetBSD.  While it held up well in simpler cases,
I noted that it was significantly slower on long format strings, which
I traced to two separate problems:

1. Our implementation always scans the format string twice, so that it
can sort out argument-ordering options (%n$).  Everybody else is bright
enough to do that only for formats that actually use %n$, and it turns
out that it doesn't really cost anything extra to do so: you can just
perform the extra scan when and if you first find a dollar specifier.
(Perhaps there's an arguable downside for this, with invalid format
strings that have non-dollar conversion specs followed by dollar ones:
with this approach we might fetch some arguments before realizing that
the format is broken.  But a wrong format can cause indefinitely bad
results already, so that seems like a pretty thin objection to me,
especially if all other implementations share the same hazard.)

2. Our implementation is shoving simple data characters in the format
out to the result buffer one at a time.  More common is to skip to the
next % as fast as possible, and then dump anything skipped over using
the string-output code path, reducing the overhead of buffer overrun
checking.

The attached patch fixes both of those things, and also does some
micro-optimization hacking to avoid loops around dopr_outch() as well
as unnecessary use of pass-by-ref arguments.  This version stacks up
pretty well against all the libraries I compared it to.  The remaining
weak spot is that floating-point conversions are consistently 30%-50%
slower than the native libraries, which is not terribly surprising
considering that our implementation involves calling the native sprintf
and then massaging the result.  Perhaps there's a way to improve that
without writing our own floating-point conversion code, but I'm not
seeing an easy way offhand.  I don't think that's a showstopper though.
This code is now faster than the native code for very many other cases,
so on average it should cause no real performance problem.

I've attached both the patch and a simple performance testbed in case
anybody wants to do their own measurements.  For reference's sake,
these are the specific test cases I looked at:

snprintf(buffer, sizeof(buffer),
 "%2$.*3$f %1$d\n",
 42, 123.456, 2);

snprintf(buffer, sizeof(buffer),
 "%.*g", 15, 123.456);

snprintf(buffer, sizeof(buffer),
 "%d %d", 15, 16);

snprintf(buffer, sizeof(buffer),
 "%10d", 15);

snprintf(buffer, sizeof(buffer),
 "%s",
 
"0123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890");

snprintf(buffer, sizeof(buffer),
 "%d 
0123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890",

snprintf(buffer, sizeof(buffer),
 "%1$d 
0123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890012345678900123456789001234567890",
 42);

A couple of other notes of interest:

* The skip-to-next-% searches could alternatively be implemented with
strchr(), although then you need a strlen() call if there isn't another %.
glibc's version of strchr() is fast enough to make that a win, but since
we're not contemplating using this atop glibc, that's not a case we care
about.  On other platforms the manual loop mostly seems to be faster.

* NetBSD seems to have a special fast path for the case that the format
string is exactly "%s".  I did not adopt that idea here, reasoning that
checking for it would add overhead to all other cases, making it probably
a net loss overall.  I'm prepared to listen to arguments otherwise,
though.  It is a common case, I just doubt it's common enough (and
other library authors seem to agree).

I'll add this to the upcoming CF.

regards, tom lane

diff --git a/src/port/snprintf.c b/src/port/snprintf.c
index 851e2ae..211ff1b 100644
*** a/src/port/snprintf.c
--- b/src/port/snprintf.c
*** flushbuffer(PrintfTarget *tar

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 08/17/2018 08:24 PM, Emre Hasegeli wrote:
>> BTW how did we end up with the regression differences? Presumably you've
>> tried that on your machine and it passed. So if we adjust the expected
>> file, won't it fail on some other machines?
> 
> I had another patch to check for -0 inside float{4,8}_{div,mul}().  I
> dropped it on the last set of patches, so the tests were broken.  I
> get -0 as a result of -x * 0 both on Mac and Linux.
> 

OK, that explains is. I won't have time to get this committed before CF
2018-09, but I'll pick it up in September.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Tom Lane]

Alexander Korotkov  writes:
> On Fri, Aug 17, 2018 at 8:38 PM Tom Lane  wrote:
>> Alexander Korotkov  writes:
>>> Yes, that's correct.  On standby read-only queries can tolerate
>>> concurrent heap truncation.

>> Uh, what???

> VACUUM truncates heap relation only after deletion of all the tuples
> from tail pages.

Right; the problem I'm worried about is possible accesses to
already-removed pages by concurrent scans.

> And in md.c we already have logic to return zeroed pages, when trying
> to read past relation in recovery.

But then you are injecting bad pages into the shared buffer arena.
In any case, depending on that behavior seems like a bad idea, because
it's a pretty questionable kluge in itself.

Another point is that the truncation code attempts to remove all
to-be-truncated-away pages from the shared buffer arena, but that only
works if nobody else is loading such pages into shared buffers
concurrently.  In the presence of concurrent scans, we might be left
with valid-looking buffers for pages that have been truncated away
on-disk.  That could cause all sorts of fun later.  Yeah, the buffers
should contain only dead tuples ... but, for example, they might not
be hinted dead.  If somebody sets one of those hint bits and then
writes the buffer back out to disk, you've got real problems.

Perhaps there's some gold to be mined by treating truncation locks
differently from other AELs, but I don't think you can just ignore
them on the standby, any more than they can be ignored on the master.

regards, tom lane

Re: [PATCH] Improve geometric types

[Tom Lane]

Emre Hasegeli  writes:
>> BTW how did we end up with the regression differences? Presumably you've
>> tried that on your machine and it passed. So if we adjust the expected
>> file, won't it fail on some other machines?

> I had another patch to check for -0 inside float{4,8}_{div,mul}().  I
> dropped it on the last set of patches, so the tests were broken.  I
> get -0 as a result of -x * 0 both on Mac and Linux.

I'll bet a good deal of money that you'll find that does not hold
true across the whole buildfarm.

regards, tom lane

Re: Pre-v11 appearances of the word "procedure" in v11 docs

[Peter Geoghegan]

On Fri, Aug 17, 2018 at 7:15 AM, Peter Eisentraut
 wrote:
> Attached are my proposed patches.

I take it that you propose all 3 for backpatch to v11?

-- 
Peter Geoghegan

Re: InsertPgAttributeTuple() and attcacheoff

[Peter Eisentraut]

On 14/08/2018 17:52, Robert Haas wrote:
> On Tue, Aug 14, 2018 at 3:50 PM, Tom Lane  wrote:
>> Peter Eisentraut  writes:
>>> It seems to me that it would make sense if InsertPgAttributeTuple() were
>>> to set attcacheoff to -1 instead of taking it from the caller.
>>
>> Looked this over, no objections.
>>
>> I wonder whether we should set that field to -1 when we *read*
>> pg_attribute rows from disk, and be less fussed about what gets written
>> out.  The only real advantage is that this'd protect us from foolish
>> manual changes to pg_attribute.attcacheoff entries, but that doesn't
>> seem negligible.
> 
> I wouldn't object to forcibly writing in -1 when we read the data, but
> I don't think it's a good idea to let values other than -1 get written
> to the disk.  User-visible random nonsense in system catalogs seems
> like too much of a foot-gun to me.

I agree.  Committed as presented then.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Alexander Korotkov]

On Fri, Aug 17, 2018 at 9:55 PM Tom Lane  wrote:
> Alexander Korotkov  writes:
> > On Fri, Aug 17, 2018 at 8:38 PM Tom Lane  wrote:
> >> Alexander Korotkov  writes:
> >>> Yes, that's correct.  On standby read-only queries can tolerate
> >>> concurrent heap truncation.
>
> >> Uh, what???
>
> > VACUUM truncates heap relation only after deletion of all the tuples
> > from tail pages.
>
> Right; the problem I'm worried about is possible accesses to
> already-removed pages by concurrent scans.
>
> > And in md.c we already have logic to return zeroed pages, when trying
> > to read past relation in recovery.
>
> But then you are injecting bad pages into the shared buffer arena.
> In any case, depending on that behavior seems like a bad idea, because
> it's a pretty questionable kluge in itself.
>
> Another point is that the truncation code attempts to remove all
> to-be-truncated-away pages from the shared buffer arena, but that only
> works if nobody else is loading such pages into shared buffers
> concurrently.  In the presence of concurrent scans, we might be left
> with valid-looking buffers for pages that have been truncated away
> on-disk.  That could cause all sorts of fun later.  Yeah, the buffers
> should contain only dead tuples ... but, for example, they might not
> be hinted dead.  If somebody sets one of those hint bits and then
> writes the buffer back out to disk, you've got real problems.
>
> Perhaps there's some gold to be mined by treating truncation locks
> differently from other AELs, but I don't think you can just ignore
> them on the standby, any more than they can be ignored on the master.

Thank you for the explanation.  I see that injecting past OEF pages
into shared buffers doesn't look good.  I start thinking about letting
caller of ReadBuffer() (or its variation) handle past OEF situation.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 08/17/2018 08:56 PM, Tom Lane wrote:
> Emre Hasegeli  writes:
>>> BTW how did we end up with the regression differences? Presumably you've
>>> tried that on your machine and it passed. So if we adjust the expected
>>> file, won't it fail on some other machines?
> 
>> I had another patch to check for -0 inside float{4,8}_{div,mul}().  I
>> dropped it on the last set of patches, so the tests were broken.  I
>> get -0 as a result of -x * 0 both on Mac and Linux.
> 
> I'll bet a good deal of money that you'll find that does not hold
> true across the whole buildfarm.
> 

Hmm, yeah. Based on past experience, the powerpc machines are likely to
stumble on this.

FWIW my understanding is that these failures actually happen in new
tests, it's not an issue introduced by this patch series.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: docs: note ownership requirement for refreshing materialized views

[Jonathan S. Katz]

> On Aug 17, 2018, at 9:21 AM, Tom Lane  wrote:
> 
> Dave Cramer  writes:
>> So it seems this patch is being ignored in this thread.
> 
> Well, Jonathan did kind of hijack what appears to be a thread about
> documentation (with an already-committed fix).

I apologize if it was interpreted as hijacking, I had brought it up in
my initial reply to Dian, as I’ve seen others do similarly on threads.

> I'd suggest reposting that patch in its own thread and adding it to
> the next CF.

I will go ahead and do this.

Thanks, and thank you for the patch Dian.

Jonathan



signature.asc
Description: Message signed with OpenPGP

Re: [HACKERS] WIP: long transactions on hot standby feedback replica / proof of concept

[Tom Lane]

Alexander Korotkov  writes:
> On Fri, Aug 17, 2018 at 9:55 PM Tom Lane  wrote:
>> Another point is that the truncation code attempts to remove all
>> to-be-truncated-away pages from the shared buffer arena, but that only
>> works if nobody else is loading such pages into shared buffers
>> concurrently.  In the presence of concurrent scans, we might be left
>> with valid-looking buffers for pages that have been truncated away
>> on-disk.  That could cause all sorts of fun later.  Yeah, the buffers
>> should contain only dead tuples ... but, for example, they might not
>> be hinted dead.  If somebody sets one of those hint bits and then
>> writes the buffer back out to disk, you've got real problems.

> Thank you for the explanation.  I see that injecting past OEF pages
> into shared buffers doesn't look good.  I start thinking about letting
> caller of ReadBuffer() (or its variation) handle past OEF situation.

That'd still have the same race condition, though: between the time
we start to drop the doomed pages from shared buffers, and the time
we actually perform ftruncate, concurrent scans could re-load such
pages into shared buffers.

Could it work to ftruncate first and flush shared buffers after?
Probably not, I think the write-back-dirty-hint-bits scenario
breaks that one.

If this were easy, we'd have fixed it years ago :-(.  It'd sure
be awfully nice not to need AEL during autovacuum, even transiently;
but I'm not sure how we get there without adding an unpleasant amount
of substitute locking in table scans.

regards, tom lane

Fix for REFRESH MATERIALIZED VIEW ownership error message

[Jonathan S. Katz]

Hi,I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW as anon-superuser or table owner yields the following message:    test=> REFRESH MATERIALIZED VIEW blah;    ERROR: must be owner of relation blahThe error message should say "...owner of materialized view..."The attached patch corrects this by setting the "relkind" for theREFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the aclcheckreturns the appropriate error message. The updated patch can be tested as such:    CREATE ROLE bar LOGIN;    CREATE TABLE a (x int);    CREATE MATERIALIZED VIEW b AS SELECT * FROM a;    \c - bar    REFRESH MATERIALIZED VIEW b;    ERROR:  must be owner of materialized view bI'm happy to generate the backpatches for it but wanted to receive feedbackfirst.Thanks,Jonathan[1] https://www.postgresql.org/message-id/55498B5B-0155-4B0E-9B97-23167F8CB380%40excoventures.com

0001-treat-refresh-mat-view-as-mat-view.patch
Description: Binary data


signature.asc
Description: Message signed with OpenPGP

Re: [PATCH] Improve geometric types

[Tom Lane]

Tomas Vondra  writes:
> Hmm, yeah. Based on past experience, the powerpc machines are likely to
> stumble on this.

> FWIW my understanding is that these failures actually happen in new
> tests, it's not an issue introduced by this patch series.

Yeah, we've definitely hit such problems before.  The geometric logic
seems particularly prone to it because it's doing more and subtler
float arithmetic than the rest of the system ... but it's not the sole
source of minus-zero issues.

regards, tom lane

Re: Fix for REFRESH MATERIALIZED VIEW ownership error message

[Alvaro Herrera]

On 2018-Aug-17, Jonathan S. Katz wrote:

> Hi,
> 
> I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW as a
> non-superuser or table owner yields the following message:
> 
> test=> REFRESH MATERIALIZED VIEW blah;
> ERROR: must be owner of relation blah
> 
> The error message should say "...owner of materialized view..."
> 
> The attached patch corrects this by setting the "relkind" for the
> REFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the aclcheck
> returns the appropriate error message. The updated patch can be tested as 
> such:
> 
> CREATE ROLE bar LOGIN;
> CREATE TABLE a (x int);
> CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
> \c - bar
> REFRESH MATERIALIZED VIEW b;
> ERROR:  must be owner of materialized view b
> 
> I'm happy to generate the backpatches for it but wanted to receive feedback
> first.

Maybe add your test to some regress/ file?

As it is cosmetic, my inclination would be not to backpatch it.
However, I don't quite see how this patch can possibly fix the problem,
since the new struct member is not used anywhere AFAICT.

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

Getting NOT NULL constraint from pg_attribute

[Wu Ivy]

Hi developers,

I’m currently building a Postgres C extension that fetch data from a Postgres 
table.
Since the table can be large, in order to prevent memory overrun, I use 
SPI_cursor_fetch to fetch chunks of data. The result rows are saved in 
SPITupleTable* SPI_tuptable and attributes are saved in SPI_tuptable->tupdesc. 
In order to process my data, I need to get information of column nullability 
(whether column has NOT NULL constrain). I can get this information by calling:

TupleDesc tupdesc = SPI_tuptable->tupdesc;
bool is_nullable = TupleDescAttr(tupdesc, column_num - 1) -> attnotnull;
However, the result (is_nullable) is always 0, meaning the column does not have 
NOT NULLl constraint, even for columns that do have the NOT NULL constraint.

Any idea of why is it happening? 
Thanks in advance!

Best,
Ivy