remove duplicated words in comments .. across lines
Resending to -hackers as I realized this isn't a documentation issue so not
appropriate or apparently interesting to readers of -doc.
Inspired by David's patch [0], find attached fixing words duplicated, across
line boundaries.
I should probably just call the algorithm proprietary, but if you really wanted
to know, I've suffered again through sed's black/slashes.
time find . -name '*.c' -o -name '*.h' |xargs sed -srn '/\/\*/!d; :l;
/\*\//!{N; b l}; s/\n[[:space:]]*\*/\n/g;
/(\<[[:alpha:]]{1,})\>\n[[:space:]]*\<\1\>/!d; s//>>&<\n[[:space:]]*\<\1\>/!d; s//>>&parent. The root page is never released, to
- * to prevent conflict with vacuum process.
+ * prevent conflict with vacuum process.
*/
static void
ginFindParents(GinBtree btree, GinBtreeStack *stack)
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 4aff6cf7f2..3f778093cb 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1737,7 +1737,7 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len)
* possible that GXACTs that were valid at checkpoint start will no longer
* exist if we wait a little bit. With typical checkpoint settings this
* will be about 3 minutes for an online checkpoint, so as a result we
- * we expect that there will be no GXACTs that need to be copied to disk.
+ * expect that there will be no GXACTs that need to be copied to disk.
*
* If a GXACT remains valid across multiple checkpoints, it will already
* be on disk so we don't bother to repeat that write.
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index 5c6de4989c..4a039b1190 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -422,7 +422,7 @@ ExecuteRecoveryCommand(const char *command, const char *commandName, bool failOn
/*
* A file was restored from the archive under a temporary filename (path),
* and now we want to keep it. Rename it under the permanent filename in
- * in pg_wal (xlogfname), replacing any existing file with the same name.
+ * pg_wal (xlogfname), replacing any existing file with the same name.
*/
void
KeepFileRestoredFromArchive(const char *path, const char *xlogfname)
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 3e148f03d0..1262594058 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -2898,7 +2898,7 @@ analyze_mcv_list(int *mcv_counts,
* significantly more common than the estimated selectivity they would
* have if they weren't in the list. All non-MCV values are assumed to be
* equally common, after taking into account the frequencies of all the
- * the values in the MCV list and the number of nulls (c.f. eqsel()).
+ * values in the MCV list and the number of nulls (c.f. eqsel()).
*
* Here sumcount tracks the total count of all but the last (least common)
* value in the MCV list, allowing us to determine the effect of excluding
diff --git a/src/backend/commands/seclabel.c b/src/backend/commands/seclabel.c
index 5ee46905d8..1ac7756f2a 100644
--- a/src/backend/commands/seclabel.c
+++ b/src/backend/commands/seclabel.c
@@ -321,7 +321,7 @@ SetSharedSecurityLabel(const ObjectAddress *object,
/*
* SetSecurityLabel attempts to set the security label for the specified
* provider on the specified object to the given value. NULL means that any
- * any existing label should be deleted.
+ * existing label should be deleted.
*/
void
SetSecurityLabel(const ObjectAddress *object,
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 1b659a5870..65f2ba85fe 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -956,7 +956,7 @@ info_cb(const SSL *ssl, int type, int args)
* precomputed.
*
* Since few sites will bother to create a parameter file, we also
- * also provide a fallback to the parameters provided by the
+ * provide a fallback to the parameters provided by the
* OpenSSL project.
*
* These values can be static (once loaded or computed) since the
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 0dd55ac1ba..d241d9b3f9 100644
--- a/src/backend/partitioning/partprune.c
+++
RE: [HACKERS] Proposal to add work_mem option to postgres_fdw module
>I have committed just the libpq change. The documentation change was >redundant, because the documentation already stated that all libpq options are >accepted. (Arguably, the documentation was wrong before.) Also, the proposed >test change didn't >seem to add much. It just checked that the foreign server >option is accepted, but not whether it does anything. If you want to develop >a more substantive test, we could consider it, but I feel that since this all >just goes to libpq, we don't need to test it >further. Thanks ! Regards. Noriyoshi Shinoda -Original Message- From: Peter Eisentraut [mailto:peter.eisentr...@2ndquadrant.com] Sent: Friday, September 7, 2018 10:17 PM To: Shinoda, Noriyoshi (PN Japan GCS Delivery) ; fabriziome...@gmail.com; [pgdg] Robert Haas ; mich...@paquier.xyz Cc: Pgsql Hackers Subject: Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module On 05/09/2018 18:46, Peter Eisentraut wrote: > On 01/09/2018 06:33, Shinoda, Noriyoshi (PN Japan GCS Delivery) wrote: >> Certainly the PQconndefaults function specifies Debug flag for the "options" >> option. >> I think that eliminating the Debug flag is the simplest solution. >> For attached patches, GUC can be specified with the following syntax. >> >> CREATE SERVER remsvr1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host >> 'host 1', ..., options '-c work_mem=64MB -c geqo=off'); ALTER SERVER >> remsv11 OPTIONS (SET options '-c work_mem=64MB -c geqo=off'); >> >> However, I am afraid of the effect that this patch will change the behavior >> of official API PQconndefaults. > > It doesn't change the behavior of the API, it just changes the result > of the API function, which is legitimate and the reason we have the > API function in the first place. > > I think this patch is fine. I'll work on committing it. I have committed just the libpq change. The documentation change was redundant, because the documentation already stated that all libpq options are accepted. (Arguably, the documentation was wrong before.) Also, the proposed test change didn't seem to add much. It just checked that the foreign server option is accepted, but not whether it does anything. If you want to develop a more substantive test, we could consider it, but I feel that since this all just goes to libpq, we don't need to test it further. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: *_to_xml() should copy SPI_processed/SPI_tuptable
Andrew Gierth writes: > "Tom" == Tom Lane writes: > Tom> Comments? > +1 to backpatching it all, including the initialization in SPI_connect. Done. regards, tom lane
Re: unaccent(text) fails depending on search_path (WAS: pg_upgrade fails saying function unaccent(text) doesn't exist)
Bruce Momjian writes: > On Fri, Sep 7, 2018 at 06:43:52PM -0400, Tom Lane wrote: >> AFAICS there are no internal-to-the-C-code search path dependencies >> in earthdistance.c, so it's not the same problem. > Uh, there is an SQL function that calls functions from the module that > fail. It would be a CREATE FUNCTION patch, I think, but I thought the > issue was the same. Not really. You could either interpolate @extschema@ into the text of the referencing function, or (though much inferior for performance) have it SET SEARCH_PATH FROM CURRENT. Either of those changes would involve an extension version bump since they're changing the extension script. What's more of a problem is that we could no longer claim the extension is relocatable. My unaccent fix dodged that by looking up the C function's current schema, but I don't think there's any equivalent functionality available at SQL level. regards, tom lane
Re: unaccent(text) fails depending on search_path (WAS: pg_upgrade fails saying function unaccent(text) doesn't exist)
On Fri, Sep 7, 2018 at 06:43:52PM -0400, Tom Lane wrote: > Bruce Momjian writes: > > If we are going down this route, is there any thought of handling > > earchdistance the same way? > > https://www.postgresql.org/message-id/20180330205229.gs8...@momjian.us > > AFAICS there are no internal-to-the-C-code search path dependencies > in earthdistance.c, so it's not the same problem. Uh, there is an SQL function that calls functions from the module that fail. It would be a CREATE FUNCTION patch, I think, but I thought the issue was the same. -- 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: pgsql: Allow extensions to install built as well as unbuilt headers.
On Thu, Sep 06, 2018 at 05:18:10PM +0100, Andrew Gierth wrote: > Yeah; I checked the other constructs I used for what version they were > added in, but that one slipped through. Fix coming shortly. For the sake of the archives, Andrew has pushed 7b6b167 to address the issue, which looks fine at quick glance. -- Michael signature.asc Description: PGP signature
Re: unaccent(text) fails depending on search_path (WAS: pg_upgrade fails saying function unaccent(text) doesn't exist)
Bruce Momjian writes: > If we are going down this route, is there any thought of handling > earchdistance the same way? > https://www.postgresql.org/message-id/20180330205229.gs8...@momjian.us AFAICS there are no internal-to-the-C-code search path dependencies in earthdistance.c, so it's not the same problem. regards, tom lane
StandbyAcquireAccessExclusiveLock doesn't necessarily
Commit 37c54863c removed the code in StandbyAcquireAccessExclusiveLock that checked the return value of LockAcquireExtended. AFAICS this was flat out wrong, because it's still passing reportMemoryError = false to LockAcquireExtended, meaning there are still cases where LOCKACQUIRE_NOT_AVAIL will be returned. In such a situation, the startup process would believe it had acquired exclusive lock even though it hadn't, with potentially dire consequences. While we could certainly put back a test there, it's not clear to me that it could do anything more useful than erroring out, at least not without largely reverting 37c54863c. So my inclination is to remove the reportMemoryError = false parameter, and just let an error happen in the unlikely situation that we hit OOM for the lock table. That would allow this code to not use LockAcquireExtended at all. Indeed, I'd be rather tempted to remove that parameter from LockAcquireExtended altogether, as I don't believe it's either particularly useful, or at all well tested, or even testable. regards, tom lane
Re: unaccent(text) fails depending on search_path (WAS: pg_upgrade fails saying function unaccent(text) doesn't exist)
On Wed, Sep 5, 2018 at 06:37:00PM -0400, Tom Lane wrote:
> [ redirecting to pgsql-hackers ]
>
> I wrote:
> > Gunnlaugur Thor Briem writes:
> >> SET search_path = "$user"; SELECT public.unaccent('foo');
> >> SET
> >> ERROR: text search dictionary "unaccent" does not exist
>
> > Meh. I think we need the attached, or something just about like it.
> >
> > It's barely possible that there's somebody out there who's relying on
> > setting the search path to allow choosing among multiple "unaccent"
> > dictionaries. But there are way more people whose functions are
> > broken due to the recent search-path-tightening changes.
>
> Here's a slightly more efficient version.
If we are going down this route, is there any thought of handling
earchdistance the same way?
https://www.postgresql.org/message-id/20180330205229.gs8...@momjian.us
--
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: buildfarm: could not read block 3 in file "base/16384/2662": read only 0 of 8192 bytes
I went ahead and pushed this, since the window for getting buildfarm testing done before Monday's wrap is closing fast. We can always improve on it later, but I think beta3 ought to carry some fix for the problem. regards, tom lane
Re: Collation versioning
On Thu, Sep 6, 2018 at 5:36 PM Thomas Munro wrote: > On Thu, Sep 6, 2018 at 12:01 PM Peter Eisentraut > wrote: > > Also, note that this mechanism only applies to collation objects, not to > > database-global locales. So most users wouldn't be helped by this approach. > > Yeah, right, that would have to work for this to be useful. I will > look into that. We could perform a check up front in (say) CheckMyDatabase(), or maybe defer until the first string comparison. The tricky question is where to store it. 1. We could add datcollversion to pg_database. 2. We could remove datcollate and datctype and instead store a collation OID. I'm not sure what problems would come up, but for starters it seems a bit weird to have a shared catalog pointing to rows in a non-shared catalog. The same question comes up if we want to support ICU as a database level default. Add datcollprovider, or point to a pg_collation row? -- Thomas Munro http://www.enterprisedb.com
Re: libpq stricter integer parsing
Hello Michael,
Hmmm. It does apply on a test I just did right know...
That's weird, it does not apply for me either with patch -p1 and there
is on conflict in fe-connect.c, which looks easy enough to fix by the
way.
Weird indeed. However, even if the patch applied cleanly for me, it was
not compiling anymore. Attached an updated version, in which I also tried
to improve the error message on trailing garbage.
--
Fabiendiff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 5e7931ba90..a6cbbea655 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1591,6 +1591,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 db5aacd0e9..5ea51d5c55 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, , 10);
+
+ if (errno != 0)
+ {
+ appendPQExpBuffer(>errorMessage,
+ libpq_gettext("invalid integer \"%s\" for keyword \"%s\": %s\n"),
+ str, context, strerror(errno));
+ return false;
+ }
+ else if (*endptr != '\0')
+ {
+ appendPQExpBuffer(>errorMessage,
+ libpq_gettext("invalid integer \"%s\" for keyword \"%s\": trailing garbage\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, , 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, , 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, , 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, , conn, "keepalives_idle"))
+ return 0;
+
+ if (conn->keepalives_interval &&
+ !strict_atoi(conn->keepalives_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 */
@@ -1817,7 +1856,9 @@ connectDBComplete(PGconn *conn)
*/
if (conn->connect_timeout != NULL)
{
- timeout = atoi(conn->connect_timeout);
+ if (!strict_atoi(conn->connect_timeout, , conn, "connect_timeout"))
+ return 0;
+
if (timeout > 0)
{
/*
@@ -1828,6 +1869,8 @@ connectDBComplete(PGconn *conn)
if (timeout < 2)
timeout = 2;
}
+ else /* negative means 0 */
+ timeout = 0;
}
for (;;)
@@ -2094,7 +2137,9 @@ keep_going: /* We will come back to here until there is
thisport = DEF_PGPORT;
else
{
- thisport = atoi(ch->port);
+ if (!strict_atoi(ch->port, , conn, "port"))
+goto error_return;
+
if (thisport < 1 || thisport > 65535)
{
appendPQExpBuffer(>errorMessage,
Re: pgsql: Refactor dlopen() support
Peter Eisentraut writes: > On 07/09/2018 16:19, Tom Lane wrote: >> Somehow or other, the changes you made in dfmgr.c's #include lines >> have made it so that find_rendezvous_variable's local "bool found" >> variable is actually of type _Bool (which is word-wide on these >> machines). > Ah because dlfcn.h includes stdbool.h. Hmm. Yeah, and that's still true as of current macOS, it seems. I can make the problem go away with the attached patch (borrowed from similar code in plperl.h). It's kind of grotty but I'm not sure there's a better way. regards, tom lane diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index c2a2572..4a5cc7c 100644 *** a/src/backend/utils/fmgr/dfmgr.c --- b/src/backend/utils/fmgr/dfmgr.c *** *** 18,24 --- 18,34 #ifdef HAVE_DLOPEN #include + + /* + * On macOS, insists on including . If we're not + * using stdbool, undef bool to undo the damage. + */ + #ifndef USE_STDBOOL + #ifdef bool + #undef bool #endif + #endif + #endif /* HAVE_DLOPEN */ #include "fmgr.h" #include "lib/stringinfo.h"
Re: libpq stricter integer parsing
On Fri, Sep 07, 2018 at 05:13:17PM +0200, Fabien COELHO wrote: > Hmmm. It does apply on a test I just did right know... That's weird, it does not apply for me either with patch -p1 and there is on conflict in fe-connect.c, which looks easy enough to fix by the way. -- Michael signature.asc Description: PGP signature
Re: pg_constraint.conincluding is useless
Bruce Momjian writes: > On Sun, Sep 2, 2018 at 01:27:25PM -0400, Tom Lane wrote: >> If we do do a bump for beta4, I'd be strongly tempted to address the >> lack of a unique index for pg_constraint as well, cf >> https://www.postgresql.org/message-id/10110.1535907...@sss.pgh.pa.us > Uh, if we add a unique index later, wouldn't that potentially cause > future restores to fail? Seems we better add it now. Yup, done already. regards, tom lane
Re: A strange GiST error message or fillfactor of GiST build
Bruce Momjian writes: > So, are we going to output a notice if a non-100% fill factor is > specified? I would not think that's helpful. regards, tom lane
Re: pg_constraint.conincluding is useless
On 2018-Sep-07, Bruce Momjian wrote: > On Sun, Sep 2, 2018 at 01:27:25PM -0400, Tom Lane wrote: > > Alvaro Herrera writes: > > > This requires a catversion bump, for which it may seem a bit late; > > > however I think it's better to release pg11 without a useless catalog > > > column only to remove it in pg12 ... > > > > Catversion bumps during beta are routine. If we had put out rc1 > > I'd say it was too late, but we have not. > > > > If we do do a bump for beta4, I'd be strongly tempted to address the > > lack of a unique index for pg_constraint as well, cf > > https://www.postgresql.org/message-id/10110.1535907...@sss.pgh.pa.us > > Uh, if we add a unique index later, wouldn't that potentially cause > future restores to fail? Seems we better add it now. Committed on Sep 4th as 17b7c302b5fc. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_constraint.conincluding is useless
On Sun, Sep 2, 2018 at 01:27:25PM -0400, Tom Lane wrote: > Alvaro Herrera writes: > > This requires a catversion bump, for which it may seem a bit late; > > however I think it's better to release pg11 without a useless catalog > > column only to remove it in pg12 ... > > Catversion bumps during beta are routine. If we had put out rc1 > I'd say it was too late, but we have not. > > If we do do a bump for beta4, I'd be strongly tempted to address the > lack of a unique index for pg_constraint as well, cf > https://www.postgresql.org/message-id/10110.1535907...@sss.pgh.pa.us Uh, if we add a unique index later, wouldn't that potentially cause future restores to fail? Seems we better add it now. -- 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: A strange GiST error message or fillfactor of GiST build
On Thu, Sep 6, 2018 at 04:16:45PM +0900, Kyotaro HORIGUCHI wrote: > Hello. > > > Just my 2 cents: that was a hacky use case for development reasons. > > I know. So I intended to preserve the parameter with default of 100% if no > one strongly objects to preserve. Then I abandoned that since I had:p So, are we going to output a notice if a non-100% fill factor is specified? -- 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: Performance improvements for src/port/snprintf.c
I benchmarked this, using your testbed and comparing to libc sprintf
(Ubuntu GLIBC 2.27-0ubuntu3) and another implementation I know [1], all
compiled with gcc 5.4.0 with -O2. I used bigger decimals in one of the
formats, but otherwise they are the same as yours. Here is the table of
conversion time relative to libc:
format pg stb
("%2$.*3$f %1$d\n", 42, 123.456, 2) 1.03 -
("%.*g", 15, 123.456) 1.08 0.31
("%10d", 15) 0.63 0.52
("%s", "012345678900123456789001234 2.06 6.20
("%d 012345678900123456789001234567 2.03 1.81
("%1$d 0123456789001234567890012345 1.34 -
("%d %d", 845879348, 994502893) 1.97 0.59
Surprisingly, our implementation is twice faster than libc on "%10d".
Stb is faster than we are with floats, but it uses its own algorithm for
that. It is also faster with decimals, probably because it uses a
two-digit lookup table, not one-digit like we do. Unfortunately it
doesn't support dollars.
1. https://github.com/nothings/stb/blob/master/stb_sprintf.h
--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Re: Incorrect error handling for two-phase state files resulting in data loss
On Fri, Aug 17, 2018 at 07:56:00AM +0900, Michael Paquier wrote: > As this is a data corruption issue, are there any objections if I patch > and back-patch? I also would like to get this stuff in first as I have > other refactoring work which would shave some more code. I looked at this patch again after letting it aside for a couple of weeks and pushed it. This is a potential, very rare data loss bug, and nobody complained about it so I have not done any back-patch. Please let me know if you would like to see something happen in stable branches as well. -- Michael signature.asc Description: PGP signature
Re: Standby reads fail when autovacuum take AEL during truncation
Hi! On Fri, Sep 7, 2018 at 3:17 PM Adrien NAYRAT wrote: > I was faced on $SUBJECT on an heavily updated table and the same table > heavily accessed on standby server. > > I notice autovacuum try to take an AEL in lazy_truncate_heap(). On > primary we try during VACUUM_TRUNCATE_LOCK_WAIT_INTERVAL (50ms) and we > failed after several attempts. > > But we do not have this mechanism on a standby, AEL could lock simple > SELECT during the RelationTruncate(). > > Please note, this can occurs even with hot_standby_feedback = on > > I wonder how we can improve this? Maybe by introducing an option to > disable truncation for autovacuum on specific table? Please, take a look at following threads: 1. https://www.postgresql.org/message-id/c9374921e50a5e8fb1ecf04eb8c6ebc3%40postgrespro.ru 2. https://www.postgresql.org/message-id/CAHGQGwE5UqFqSq1%3DkV3QtTUtXphTdyHA-8rAj4A%3DY%2Be4kyp3BQ%40mail.gmail.com -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [PATCH] Fix for infinite signal loop in parallel scan
Hi, On 2018-09-07 17:57:05 +0200, Chris Travers wrote: > Attached is the patch we are fully testing at Adjust. For the future, please don't start a separate threads from the bugreport / original discussion. Makes it much harder to follow. Greetings, Andres Freund
Re: pgsql: Refactor dlopen() support
On 07/09/2018 16:19, Tom Lane wrote: > Somehow or other, the changes you made in dfmgr.c's #include lines > have made it so that find_rendezvous_variable's local "bool found" > variable is actually of type _Bool (which is word-wide on these > machines). However, hash_search thinks its output variable is > of type pointer to "typedef char bool". The proximate cause of > the observed failure is that find_rendezvous_variable sees "found" > as true when it should not, and thus fails to zero out the variable's > value. Ah because dlfcn.h includes stdbool.h. Hmm. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Hint to set owner for tablespace directory
On 09/07/2018 06:05 PM, Peter Eisentraut wrote: On 07/09/2018 11:59, Maksim Milyutin wrote: OK. However I think it would be helpful to leave the mention about setting necessary owner for tablespace directory. My final version of hint is "You might need to fix permissions on this directory or its parents or install the PostgreSQL system user as the owner of this directory." And updated version of patch is attached. You are still proposing to hint that the permissions on the tablespace directory might be correct but that the main database instance is running under the wrong user. Not exactly this way. The case that motivated me to make this patch was the novice user after installing postgres from package (with setting up postgres user and initializing PGDATA with right permissions) tried to create the necessary tablespace directories from himself (the owner of those directories was that user). The error message "could not set permissions on directory ..." disoriented that user. The need to change the owner of those directories came after careful reading of documentation. I think it would be helpful to show the proposed hint to more operatively resolve the problem. -- Regards, Maksim Milyutin
[PATCH] Fix for infinite signal loop in parallel scan
Hi; Attached is the patch we are fully testing at Adjust. There are a few non-obvious aspects of the code around where the patch hits.I have run make check on Linux and MacOS, and make check-world on Linux (check-world fails on MacOS on all versions and all branches due to ecpg failures). Automatic tests are difficult because it is to a rare race condition which is difficult (or possibly impossible) to automatically create. Our current approach testing is to force the issue using a debugger. Any ideas on how to reproduce automatically are appreciated but even on our heavily loaded systems this seems to be a very small portion of queries that hit this case (meaning the issue happens about once a week for us). The main problem is that under certain rare circumstances we see recovery conflicts going into loops sending sigusr1 to the parallel query which retries a posix_falloc call. The call gets interrupted by the signal perpetually and the replication cannot proceed. The patch attempts to handle cases where we are trying to cancel the query or terminate the process in the same way we would handle an ENOSPC in the resize operation, namely to break off the loop, do relevant cleanup, and then throw relevant exceptions. There is a very serious complication in this area, however, which is that dsm_impl_op takes an elevel parameter which determines whether or not it is safe to throw errors. This elevel is then passed to ereport inside the function, and this function also calls the resize operation itself. Since this is not safe with CHECK_FOR_INTERRUPTS(), I conditionally do this only if elevel is greater or equal to ERROR. Currently all codepaths here use elevel ERROR when reaching this path but given that the calling function supports semantics where this could be lower, and where a caller might have additional cleanup to do, I don't think one can just add CHECK_FOR_INTERRUPTS() there even though that would work for now since this could create all kinds of subtle bugs in the future. So what the patch does is check for whether we are trying to end the query or the backend and does not retry the resize operation if either of those conditions are true. In those cases it will set errno to EINTR and return the same. The only caller then, if the resize operation failed, checks for an elevel greater or equal to ERROR, and whether the errno is set to EINTR. If so it checks for signals. If these do not abort the query, we then fall through and pass the ereport with the supplied elevel as we would have otherwise, and return false to the caller. In current calls to this function, this means that interrupts are handled right after cleanup of the dynamic shared memory and these then abort the query or exit the backend. Future calls could specify a WARNING elevel if they have extra cleanup to do, in which case signals would not be checked, and the same warning found today would found in the log. At the next CHECK_FOR_INTERRUPTS() call, the appropriate errors would be raised etc. In the past it had been suggested we do PG_TRY(); and PG_CATCH(), but given that it is not consistent whether we can raise an error or whether we MUST raise an error, I don't see how this approach can work. As far as I can see, we MUST raise an error in the appropriate spot if and only if elevel is set to a sufficient level. Backporting this is pretty trivial. We expect to confirm this ourselves on both master and 10. I can prepare back ports fairly quickly. Is there any feedback on this approach before I add it to the next commitfest? -- Best Regards, Chris Travers Head of Database Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com Saarbrücker Straße 37a, 10405 Berlin racecond.patch Description: Binary data
Re: Removing useless \. at the end of copy in pgbench
Yeah, chances are that someone is going to make a change that will require for example 8.4, and nobody would update this because the differences between 8.2 and 8.4 are long forgotten. It seems there is more enthusiasm on the side of not documenting it, so I'll close this commit fest entry. Fine with me. We spent time collecting this information, though. -- Fabien.
Re: libpq stricter integer parsing
Hello Peter, 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. This looks useful and the patch looks reasonable, but it doesn't apply anymore. Can you send in an update? Hmmm. It does apply on a test I just did right know... Usually it does not apply with "git apply" if your MUA does not know that MIME requires CR LF eol on text files, and that it is its responsability to switch to something else if desired. Pg automatic patch checker does not know about that so complains unduly. Some MUA send attachements which violates MIME (text attachements with LF only eol), thus hidding the issue. Otherwise, the "patch" command should work? -- Fabien.
Re: Hint to set owner for tablespace directory
On 07/09/2018 11:59, Maksim Milyutin wrote: > OK. However I think it would be helpful to leave the mention about > setting necessary owner for tablespace directory. My final version of > hint is "You might need to fix permissions on this directory or its > parents or install the PostgreSQL system user as the owner of this > directory." And updated version of patch is attached. You are still proposing to hint that the permissions on the tablespace directory might be correct but that the main database instance is running under the wrong user. I don't think that is a plausible situation. Clearly changing the tablespace directory permissions is much simpler than changing the entire database setup. The normal error situation here is that the tablespace directory has been created in a way that the "postgres" user can't access the way it wants. I don't see how the existing messaging is insufficient in that case. If anything, we should look to create some consistency with initdb. It basically does the same thing when it creates a new data directory. I'm not aware of a need to add more hints there either. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] proposal: schema variables
2018-09-07 14:34 GMT+02:00 Fabien COELHO : > > Hello Pavel, > > here is updated patch - I wrote some transactional support >> >> I am not sure how these new features are understandable and if these >> features does it better or not. >> > > There are possibility to reset to default value when >> >> a) any transaction is finished - the scope of value is limited by >> transaction >> >> CREATE VARIABLE foo int ON TRANSACTION END RESET; >> > > With this option I understand that it is a "within a transactionnal" > variable, i.e. when the transaction ends, whether commit or rollback, the > variable is reset to a default variable. It is not really a "session" > variable anymore, each transaction has its own value. > yes, the correct name should be "schema variable with transaction scope". I think it can be useful like short life global variable. These variables can works like transaction caches. > -- begin session > -- foo has default value, eg NULL > BEGIN; > LET foo = 1; > COMMIT/ROLLBACK; > -- foo has default value again, NULL > > b) when transaction finished by rollback >> >> CREATE VARIABLE foo int ON ROLLBACK RESET >> > > That is a little bit safer and you are back to a SESSION-scope variable, > which is reset to the default value if the (any) transaction fails? > > -- begin session > -- foo has default value, eg NULL > BEGIN; > LET foo = 1; > COMMIT; > -- foo has value 1 > BEGIN; > -- foo has value 1... > ROLLBACK; > -- foo has value NULL > > c) A more logical (from a transactional point of view - but not necessary > simple to implement, I do not know) feature/variant would be to reset the > value to the one it had at the beginning of the transaction, which is not > necessarily the default. > > -- begin session > -- foo has default value, eg NULL > BEGIN; > LET foo = 1; > COMMIT; > -- foo has value 1 > BEGIN; > LET foo = 2; (*) > -- foo has value 2 > ROLLBACK; > -- foo has value 1 back, change (*) has been reverted > > Now, when I am thinking about it, the @b is simple, but not too practical - >> when some fails, then we lost a value (any transaction inside session can >> fails). >> > > Indeed. > > The @a has sense - the behave is global value (what is not possible >> in Postgres now), but this value is destroyed by any unhandled exceptions, >> and it cleaned on transaction end. The @b is just for information and for >> discussion, but I'll remove it - because it is obscure. >> > > Indeed. > > The open question is syntax. PostgreSQL has already ON COMMIT xxx . It is >> little bit unclean, because it has semantic "on transaction end", but if I >> didn't implement @b, then ON COMMIT syntax can be used. >> > > I was more arguing on the third (c) option, i.e. on rollback the value is > reverted to its value at the beginning of the rollbacked transaction. > > At the minimum, ISTM that option (b) is enough to implement the audit > pattern, but it would mean that any session which has a rollback, for any > reason (deadlock, serialization...), would have to be reinitialized, which > would be a drawback. > > The to options could be non-transactional session variables "ON ROLLBACK > DO NOT RESET/DO NOTHING", and somehow transactional session variables "ON > ROLLBACK RESET TO DEFAULT" (b) or "ON ROLLBACK RESET TO INITIAL" (c). > @b is hardly understandable for not trained people, because any rollback in session does reset. But people expecting @c, or some near @c. I understand so you talked about @c. Now I think so it is possible to implement, but it is not trivial. The transactional behave have to calculate not only with transactions, but with SAVEPOINTS and ROLLBACK TO savepoints. On second hand, the implementation will be relatively compact. I'll hold it in my memory, but there are harder issues (support for parallelism). Regards Pavel > -- > Fabien. > >
Re: pgsql: Refactor dlopen() support
Peter Eisentraut writes: > On 07/09/2018 08:30, Tom Lane wrote: >> Buildfarm member locust doesn't like this much. I've been able to >> reproduce the problem on an old Mac laptop running the same macOS release, >> viz 10.5.8. (Note that we're not seeing it on earlier or later releases, >> which is odd in itself.) > Nothing should have changed on macOS except that the intermediate > functions pg_dl*() were replaced by direct calls to dl*(). Very strange. Somehow or other, the changes you made in dfmgr.c's #include lines have made it so that find_rendezvous_variable's local "bool found" variable is actually of type _Bool (which is word-wide on these machines). However, hash_search thinks its output variable is of type pointer to "typedef char bool". The proximate cause of the observed failure is that find_rendezvous_variable sees "found" as true when it should not, and thus fails to zero out the variable's value. No time to look further right now, but there's something rotten about the way we're handling bool. regards, tom lane
Re: Removing useless \. at the end of copy in pgbench
On 30/08/2018 08:39, Peter Eisentraut wrote: > On 29/08/2018 21:48, Andres Freund wrote: >> I'd vote for not adding it. It seems almost guaranteed to get out of >> date without anybody noticing so. Maybe that's overly pragmatic, but I >> really can't see the harm of not documenting which precise ancient >> version pgbench doesn't support anymore... > > Yeah, chances are that someone is going to make a change that will > require for example 8.4, and nobody would update this because the > differences between 8.2 and 8.4 are long forgotten. It seems there is more enthusiasm on the side of not documenting it, so I'll close this commit fest entry. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: libpq stricter integer parsing
On 17/08/2018 12:13, Fabien COELHO wrote: >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. This looks useful and the patch looks reasonable, but it doesn't apply anymore. Can you send in an update? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: libpq debug log
On 04/09/2018 02:29, Iwata, Aya wrote: > Since I'd like to monitor the information the server and the client exchange, > I think monitoring protocol messages is good. > > When a slow query is occurs, we check this client side trace log. > The purpose of this log acquisition I thought is to identify where is the > problem: > server side, application side or traffic. > And if the problem is in application side, checking the trace log to identify > what is the problem. Between perf/systemtap/dtrace and wireshark, you can already do pretty much all of that. Have you looked at those and found anything missing? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module
On 05/09/2018 18:46, Peter Eisentraut wrote: > On 01/09/2018 06:33, Shinoda, Noriyoshi (PN Japan GCS Delivery) wrote: >> Certainly the PQconndefaults function specifies Debug flag for the "options" >> option. >> I think that eliminating the Debug flag is the simplest solution. >> For attached patches, GUC can be specified with the following syntax. >> >> CREATE SERVER remsvr1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host 'host >> 1', ..., options '-c work_mem=64MB -c geqo=off'); >> ALTER SERVER remsv11 OPTIONS (SET options '-c work_mem=64MB -c geqo=off'); >> >> However, I am afraid of the effect that this patch will change the behavior >> of official API PQconndefaults. > > It doesn't change the behavior of the API, it just changes the result of > the API function, which is legitimate and the reason we have the API > function in the first place. > > I think this patch is fine. I'll work on committing it. I have committed just the libpq change. The documentation change was redundant, because the documentation already stated that all libpq options are accepted. (Arguably, the documentation was wrong before.) Also, the proposed test change didn't seem to add much. It just checked that the foreign server option is accepted, but not whether it does anything. If you want to develop a more substantive test, we could consider it, but I feel that since this all just goes to libpq, we don't need to test it further. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] proposal: schema variables
Hello Pavel, here is updated patch - I wrote some transactional support I am not sure how these new features are understandable and if these features does it better or not. There are possibility to reset to default value when a) any transaction is finished - the scope of value is limited by transaction CREATE VARIABLE foo int ON TRANSACTION END RESET; With this option I understand that it is a "within a transactionnal" variable, i.e. when the transaction ends, whether commit or rollback, the variable is reset to a default variable. It is not really a "session" variable anymore, each transaction has its own value. -- begin session -- foo has default value, eg NULL BEGIN; LET foo = 1; COMMIT/ROLLBACK; -- foo has default value again, NULL b) when transaction finished by rollback CREATE VARIABLE foo int ON ROLLBACK RESET That is a little bit safer and you are back to a SESSION-scope variable, which is reset to the default value if the (any) transaction fails? -- begin session -- foo has default value, eg NULL BEGIN; LET foo = 1; COMMIT; -- foo has value 1 BEGIN; -- foo has value 1... ROLLBACK; -- foo has value NULL c) A more logical (from a transactional point of view - but not necessary simple to implement, I do not know) feature/variant would be to reset the value to the one it had at the beginning of the transaction, which is not necessarily the default. -- begin session -- foo has default value, eg NULL BEGIN; LET foo = 1; COMMIT; -- foo has value 1 BEGIN; LET foo = 2; (*) -- foo has value 2 ROLLBACK; -- foo has value 1 back, change (*) has been reverted Now, when I am thinking about it, the @b is simple, but not too practical - when some fails, then we lost a value (any transaction inside session can fails). Indeed. The @a has sense - the behave is global value (what is not possible in Postgres now), but this value is destroyed by any unhandled exceptions, and it cleaned on transaction end. The @b is just for information and for discussion, but I'll remove it - because it is obscure. Indeed. The open question is syntax. PostgreSQL has already ON COMMIT xxx . It is little bit unclean, because it has semantic "on transaction end", but if I didn't implement @b, then ON COMMIT syntax can be used. I was more arguing on the third (c) option, i.e. on rollback the value is reverted to its value at the beginning of the rollbacked transaction. At the minimum, ISTM that option (b) is enough to implement the audit pattern, but it would mean that any session which has a rollback, for any reason (deadlock, serialization...), would have to be reinitialized, which would be a drawback. The to options could be non-transactional session variables "ON ROLLBACK DO NOT RESET/DO NOTHING", and somehow transactional session variables "ON ROLLBACK RESET TO DEFAULT" (b) or "ON ROLLBACK RESET TO INITIAL" (c). -- Fabien.
Standby reads fail when autovacuum take AEL during truncation
Hello hackers, I was faced on $SUBJECT on an heavily updated table and the same table heavily accessed on standby server. I notice autovacuum try to take an AEL in lazy_truncate_heap(). On primary we try during VACUUM_TRUNCATE_LOCK_WAIT_INTERVAL (50ms) and we failed after several attempts. But we do not have this mechanism on a standby, AEL could lock simple SELECT during the RelationTruncate(). Please note, this can occurs even with hot_standby_feedback = on I wonder how we can improve this? Maybe by introducing an option to disable truncation for autovacuum on specific table? Thanks!
Re: [HACKERS] [PROPOSAL] Temporal query processing with range types
On 01/26/2018 07:55 AM, Peter Moser wrote: We have now a new approach to plan and execute NORMALIZE as a special join node of type NORMALIZE, an append-plan on the inner join path, and a merge-join executor. For the latter, we would need to extend nodeMergejoin.c with an point-in-range-containment join. We are ready with a new prototype for the temporal NORMALIZE operation. In this prototype we do not rewrite queries as in the previous patch, but have one executor node, that solves the normalize operation. This executor is based on a merge-join. Our new patch is based on top of 75f7855369ec56d4a8e7d6eae98aff1182b85cac from September 6, 2018. The syntax is SELECT * FROM (r NORMALIZE s USING() WITH(period_r, period_s)) c; It currently is only implemented for empty USING clauses, and solely int4range as range attributes. Example: A=# table r; a | b | period_r ---+---+-- a | B | [1,7) b | B | [3,9) c | G | [8,10) (3 rows) A=# table s; c | d | period_s ---+---+-- 1 | B | [2,5) 2 | B | [3,4) 3 | B | [7,9) (3 rows) A=# SELECT * FROM (r NORMALIZE s USING() WITH(period_r, period_s)) c; period_r | a | b --+---+--- [1,2)| a | B [2,3)| a | B [3,4)| a | B [4,5)| a | B [5,7)| a | B [3,4)| b | B [4,5)| b | B [5,7)| b | B [7,9)| b | B (9 rows) A=# EXPLAIN SELECT * FROM (r NORMALIZE s USING() WITH(period_r, period_s)) c; QUERY PLAN -- Result (cost=2.15..2.22 rows=3 width=18) -> Merge ??? Join (cost=2.15..2.23 rows=3 width=22) Merge Cond: (r.period_r @> (range_split(s.period_s))) -> Sort (cost=1.05..1.06 rows=3 width=18) Sort Key: r.period_r -> Seq Scan on r (cost=0.00..1.03 rows=3 width=18) -> Sort (cost=1.09..1.10 rows=3 width=4) Sort Key: (range_split(s.period_s)) USING < -> ProjectSet (cost=0.00..1.07 rows=3 width=4) -> Seq Scan on s (cost=0.00..1.03 rows=3 width=14) (10 rows) That is, we create a new join path within sort_inner_and_outer (joinpath.c). First two projection nodes to extract the start- and end-timepoints of the range type used as interval, and above an append-plan to merge both subplans. In detail, outer_path is just sort, whereas inner_path is append of (B, ts) projection with (B, te) projection. We changed this implementation and use a set-returning function called "range_split", that extracts the upper and lower bound of a range and returns two tuples. For instance, a tuple '[4,10),a' becomes two tuples of the form '4,a' and '10,a'. Hereby, B is a set of non-temporal attributes used in join equality predicates, and [ts,te) forms the valid-time interval. Non-equality predicates must be handled separately as a filter step. The current prototype supports only an integer range-type without any additional non-temporal attributes (empty USING clause). Do you think, it is a good idea to extend the sort_inner_and_outer() with a new branch, where jointype == NORMALIZE and add the projection and append sub-paths there? We actually extended sort_inner_and_outer now. It is an early solution, to support discussions. Please see the two sections starting with "if (jointype == JOIN_TEMPORAL_NORMALIZE)" inside sort_inner_and_outer: The purpose of these sections is to change the inner path's range type into its single bounds. We accomplish this with a new function called range_split. We take the inner clause and extract the second operator of an RANGE_EQ expression out of it. We assume *for this prototype*, that their is only one such operator and that it is solely used for NORMALIZE. Then, we replace it with range_split. A range split returns a set of tuples, hence we add a new "set projection path" above the inner path, and another sort path above that. What we like to discuss now is: - Is sort_inner_and_outer the correct place to perform this split? - How could we support OID_RANGE_ELEM_CONTAINED_OP for a NORMALIZE mergejoin executor? If we use RANGE_ELEM_CONTAINED as operator, it is not an equality operator, but if we use RANGE_EQ it assumes that the right-hand-side of the operator must be a range as well. - Should we better change our mergeclause to a RANGE_ELEM_CONTAINED comparison, or keep RANGE_EQ and fix pathkeys later? - How do we update equivalence classes, pathkeys, and any other struct, when changing the inner relation's data type from "int4range" to "int" in the query tree inside "sort_inner_and_outer" to get the correct ordering and data types Best regards, Anton, Johann, Michael, Peter diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c index 5e52b90c00..ce4ffa992f 100644 ---
Re: Collation versioning
Fwiw, I was doing some tests with LC_COLLATE last year: https://github.com/ChristophBerg/lc_collate_testsuite Iirc the outcome was that everything except de_DE.UTF-8 was stable. Christoph
cache lookup failed for constraint when alter table referred by partition table
Hi, I am getting cache lookup failed for constraint error on master and 11beta3 with below test case. [edb@localhost bin]$ ./psql postgres psql (11beta3) Type "help" for help. postgres=# CREATE TABLE non_part (a INT,PRIMARY KEY(a)); CREATE TABLE postgres=# CREATE TABLE part (a INT REFERENCES non_part(a)) PARTITION BY RANGE(a); CREATE TABLE postgres=# CREATE TABLE part_p1 PARTITION OF part FOR VALUES FROM (MINVALUE) TO (MAXVALUE); CREATE TABLE postgres=# ALTER TABLE non_part ALTER COLUMN a TYPE bigint; *ERROR: cache lookup failed for constraint 16398* Thanks & Regards, Rajkumar Raghuwanshi QMG, EnterpriseDB Corporation
Re: Hint to set owner for tablespace directory
On 08/31/2018 04:59 PM, Tom Lane wrote:
Maksim Milyutin writes:
30.08.2018 19:52, Peter Eisentraut wrote:
I think the hint is backwards. When you don't have permission to chmod
the tablespace directory, you probably want to fix the permissions on
the tablespace directory or its parent.
In this case I propose to:
- replace my initial hint message to the guess what to do if errno ==
EPERM, smth like "You might need to install the PostgreSQL system user
as the owner of this directory";
- add another hint if errno == EACCES: "Fix permissions on the parent
directories".
I agree with what I think Peter is saying: the hint should just recommend
fixing permissions on the directory, for either errno code. The more
detail you try to give, the more likely the hint will be wrong
... especially when you're dealing with errno codes that aren't all that
well standardized, as these aren't.
OK. However I think it would be helpful to leave the mention about
setting necessary owner for tablespace directory. My final version of
hint is "You might need to fix permissions on this directory or its
parents or install the PostgreSQL system user as the owner of this
directory." And updated version of patch is attached.
--
Regards, Maksim Milyutin
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index f7e9160..1478462 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -585,10 +585,15 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid)
InRecovery ? errhint("Create this directory for the tablespace before "
"restarting the server.") : 0));
else
+ {
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not set permissions on directory \"%s\": %m",
- location)));
+ location),
+ errhint("You might need to fix permissions on this "
+ "directory or its parents or install the PostgreSQL "
+ "system user as the owner of this directory.")));
+ }
}
if (InRecovery)
Re: pgsql: Refactor dlopen() support
On 07/09/2018 08:30, Tom Lane wrote: > Peter Eisentraut writes: >> Refactor dlopen() support > > Buildfarm member locust doesn't like this much. I've been able to > reproduce the problem on an old Mac laptop running the same macOS release, > viz 10.5.8. (Note that we're not seeing it on earlier or later releases, > which is odd in itself.) Nothing should have changed on macOS except that the intermediate functions pg_dl*() were replaced by direct calls to dl*(). Very strange. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Use C99 designated initializers for some structs
On 30/08/2018 22:14, Andres Freund wrote: > I think we should have as rules: > > 1) Members should be defined in the same order as in the struct, that's >the requirement C++ standard is going to impose. Think it's also >reasonable stylistically. > 2) It's OK to omit setting members if zero-initialization obviously is >correct. It seems like most people were OK with that, so I committed the patch. This is something that we'll likely gain more experience with over time. > We probably should also check how well pgindent copes, and whether that > dictates some formatting choices. The patch I submitted was run through pgindent. I did not experience any problem, and it didn't reformat anything about what I had originally typed in (except one comment I think). -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Unused argument from execute_sql_string()
Hi,
I found that a argument "filename" is not used in execute_sql_string()
although the comment says "filename is used only to report errors.",
so I think we can remove this argument as done in the patch I attached.
Regards,
--
Yugo Nagata
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 2e4538146d..2d761a5773 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -683,8 +683,6 @@ read_extension_script_file(const ExtensionControlFile *control,
/*
* Execute given SQL string.
*
- * filename is used only to report errors.
- *
* Note: it's tempting to just use SPI to execute the string, but that does
* not work very well. The really serious problem is that SPI will parse,
* analyze, and plan the whole string before executing any of it; of course
@@ -694,7 +692,7 @@ read_extension_script_file(const ExtensionControlFile *control,
* could be very long.
*/
static void
-execute_sql_string(const char *sql, const char *filename)
+execute_sql_string(const char *sql)
{
List *raw_parsetree_list;
DestReceiver *dest;
@@ -921,7 +919,7 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
/* And now back to C string */
c_sql = text_to_cstring(DatumGetTextPP(t_sql));
- execute_sql_string(c_sql, filename);
+ execute_sql_string(c_sql);
}
PG_CATCH();
{
Small performance tweak to run-time partition pruning
While reviewing some other patches to improve partitioning performance
I noticed that one of the loops in ExecFindInitialMatchingSubPlans()
could be coded a bit more efficiently. The current code loops over
all the original subplans checking if the subplan is newly pruned, if
it is, the code sets the new_subplan_indexes array element to -1, else
it sets it assigns the new subplan index. This can be done more
efficiently if we make this array 1-based and initialise the whole
thing to 0 then just loop over the non-pruned subplans instead of all
subplans. Pruning all but 1 subplan is quite common.
In profiles, I'd seen ExecFindInitialMatchingSubPlans() consume about
5.2% percent of CPU time. With the patch that dropped to 0.72%.
A quick test with just 300 partitions shows about a 2.3% performance
improvement. Hardly groundbreaking, but it seems like a small enough
change for it to be worth it.
The test was conducted as follows:
postgresql.conf:
plan_cache_mode = 'force_generic_plan'
max_parallel_workers_per_gather = 0
setup:
CREATE TABLE partbench (id BIGINT NOT NULL, i1 INT NOT NULL, i2 INT
NOT NULL, i3 INT NOT NULL, i4 INT NOT NULL, i5 INT NOT NULL) PARTITION
BY RANGE (id);
\o /dev/null
select 'CREATE TABLE partbench' || x::text || ' PARTITION OF partbench
FOR VALUES FROM (' || (x*10)::text || ') TO (' ||
((x+1)*10)::text || ');' from generate_Series(0,299) x;
\gexec
\o
select.sql:
\set p_id 2999
select * from partbench where id = :p_id;
Test:
$ pgbench -n -f select.sql -M prepared -T 60 postgres
Unpatched:
tps = 6946.940678 (excluding connections establishing)
tps = 6913.993655 (excluding connections establishing)
tps = 6854.693214 (excluding connections establishing)
Patched
tps = 7066.854267 (excluding connections establishing)
tps = 7082.890458 (excluding connections establishing)
tps = 7052.255429 (excluding connections establishing)
Patch attached. I'll park this here until the November 'fest.
I've also included an additional test to ensure the other_subplans
gets updated correctly. The other tests for this seem to only perform
run-time pruning during init plan and do no further pruning, so don't
fully test that other_subplans gets updated correctly.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
v1-0001-Improve-performance-of-run-time-partition-pruning.patch
Description: Binary data
Re: pgsql: Refactor dlopen() support
Peter Eisentraut writes: > Refactor dlopen() support Buildfarm member locust doesn't like this much. I've been able to reproduce the problem on an old Mac laptop running the same macOS release, viz 10.5.8. (Note that we're not seeing it on earlier or later releases, which is odd in itself.) According to my machine, the crash is happening here: #0 _PG_init () at plpy_main.c:98 98 *plpython_version_bitmask_ptr |= (1 << PY_MAJOR_VERSION); and the reason is that the rendezvous variable sometimes contains garbage. Most sessions correctly see it as initially zero, but sometimes it contains (gdb) p plpython_version_bitmask_ptr $1 = (int *) 0x1d and I've also seen (gdb) p plpython_version_bitmask_ptr $1 = (int *) 0x7f7f7f7f It's mostly repeatable but not completely so: the 0x1d case seems to come up every time through the plpython_do test, but I don't always see the 0x7f7f7f7f case. (Maybe that's a timing artifact? It takes a variable amount of time to recover from the first crash in plpython_do, so the rest of the plpython test run isn't exactly operating in uniform conditions.) No idea what's going on here, and I'm about out of steam for tonight. regards, tom lane
