Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
Hi Palle, thanks for the extremely quick response! In that case I will include the patch in Postgres.app. Missing support for per-column collations is preferable to missing support for the standard locale! I'll have a look at the per-column collation support, it would be great if PostgreSQL on OS X would work out of the box at some point. Best regards, Jakob Am 26.11.2014 um 08:41 schrieb Palle Girgensohn gir...@pingpong.net: Hi! This is indeed a very well tested patch as we've run it in production for 8+ years on 20+ systems. It is not included upstreams mainly because I did ask for it to happen. I've been aiming to do it but haven't got around to it. Also, since 9.2 (?) there is support in PostgreSQL for setting collate locale per column. This is not yet supported by the patch, which makes it non-complete. You could argue that this is not as important as supporting the primary locale, but it would be hard to argue about that, it would have to be added for it to reach inclusion upstreams. So, I can vouch for it, it does the job just fine. Upstreams support will happen eventually. Palle 26 nov 2014 kl. 08yt:31 skrev Jakob Egger ja...@eggerapps.at mailto:ja...@eggerapps.at: When packaging PostgreSQL for Postgres.app, I discovered a problem: strcoll doesn't work for multibyte encodings on OS X. As a consequence, text sorting in PostgreSQL doesn't work. The only workaround seemed to be to use a legacy encoding like latin1, which is inacceptable. I discovered that OS X shares this limitation with FreeBSD, and there exists a patch written by Palle Girgensohn that uses the ICU library for collating strings instead of the std-c strcoll function. You can find it at http://people.freebsd.org/~girgen/postgresql-icu/README.html http://people.freebsd.org/~girgen/postgresql-icu/README.html I applied the patch, and according to preliminary testing with 9.4rc1 it seems to work flawlessly on OS X as well. See https://github.com/PostgresApp/PostgresApp/releases/tag/9.4rc1 https://github.com/PostgresApp/PostgresApp/releases/tag/9.4rc1 I have two questions: 1) Does anybody else have experience with this patch? Is it safe to release PostgreSQL binaries with this patch applied to the public? 2) Is there a reason why this patch hasn't been merged into core over the years? Since it requires setting a configure switch (--with-icu) it shouldn't break anything? Best regards, Jakob Egger
Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes
So, Here are reworked patches for the whole set, with the following changes:
- Found why replay was failing, xlogreader.c took into account BLCKSZ
- hole while it should have taken into account the compressed data
length when fetching a compressed block image.
- Reworked pglz portion to have it return status errors instead of
simple booleans. pglz stuff is as well moved to src/common as Alvaro
suggested.
I am planning to run some tests to check how much compression can
reduce WAL size with this new set of patches. I have been however able
to check that those patches pass installcheck-world with a standby
replaying the changes behind. Feel free to play with those patches...
Regards,
--
Michael
From d000aa5a5f6354b93a89d301e97985f1b29393c3 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Tue, 25 Nov 2014 14:05:59 +0900
Subject: [PATCH 1/2] Move pg_lzcompress.c to src/common
Exposing compression and decompression APIs of pglz makes possible its
use by extensions and contrib modules. pglz_decompress contained a call
to elog to emit an error message in case of corrupted data. This function
is changed to return a status code to let its callers return an error
instead. Compression function is changed similarly to make the whole set
consistent.
---
src/backend/access/heap/tuptoaster.c | 11 +-
src/backend/utils/adt/Makefile| 4 +-
src/backend/utils/adt/pg_lzcompress.c | 779 -
src/common/Makefile | 3 +-
src/common/pg_lzcompress.c| 784 ++
src/include/utils/pg_lzcompress.h | 19 +-
src/tools/msvc/Mkvcbuild.pm | 3 +-
7 files changed, 813 insertions(+), 790 deletions(-)
delete mode 100644 src/backend/utils/adt/pg_lzcompress.c
create mode 100644 src/common/pg_lzcompress.c
diff --git a/src/backend/access/heap/tuptoaster.c b/src/backend/access/heap/tuptoaster.c
index ce44bbd..9af456f 100644
--- a/src/backend/access/heap/tuptoaster.c
+++ b/src/backend/access/heap/tuptoaster.c
@@ -142,7 +142,8 @@ heap_tuple_untoast_attr(struct varlena * attr)
attr = (struct varlena *) palloc(PGLZ_RAW_SIZE(tmp) + VARHDRSZ);
SET_VARSIZE(attr, PGLZ_RAW_SIZE(tmp) + VARHDRSZ);
- pglz_decompress(tmp, VARDATA(attr));
+ if (pglz_decompress(tmp, VARDATA(attr)) != PGLZ_OK)
+elog(ERROR, compressed data is corrupted);
pfree(tmp);
}
}
@@ -167,7 +168,8 @@ heap_tuple_untoast_attr(struct varlena * attr)
attr = (struct varlena *) palloc(PGLZ_RAW_SIZE(tmp) + VARHDRSZ);
SET_VARSIZE(attr, PGLZ_RAW_SIZE(tmp) + VARHDRSZ);
- pglz_decompress(tmp, VARDATA(attr));
+ if (pglz_decompress(tmp, VARDATA(attr)) != PGLZ_OK)
+ elog(ERROR, compressed data is corrupted);
}
else if (VARATT_IS_SHORT(attr))
{
@@ -239,7 +241,8 @@ heap_tuple_untoast_attr_slice(struct varlena * attr,
preslice = (struct varlena *) palloc(size);
SET_VARSIZE(preslice, size);
- pglz_decompress(tmp, VARDATA(preslice));
+ if (pglz_decompress(tmp, VARDATA(preslice)) != PGLZ_OK)
+ elog(ERROR, compressed data is corrupted);
if (tmp != (PGLZ_Header *) attr)
pfree(tmp);
@@ -1253,7 +1256,7 @@ toast_compress_datum(Datum value)
* we insist on a savings of more than 2 bytes to ensure we have a gain.
*/
if (pglz_compress(VARDATA_ANY(DatumGetPointer(value)), valsize,
- (PGLZ_Header *) tmp, PGLZ_strategy_default)
+ (PGLZ_Header *) tmp, PGLZ_strategy_default) == PGLZ_OK
VARSIZE(tmp) valsize - 2)
{
/* successful compression */
diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index 3ea9bf4..20e5ff1 100644
--- a/src/backend/utils/adt/Makefile
+++ b/src/backend/utils/adt/Makefile
@@ -25,8 +25,8 @@ OBJS = acl.o arrayfuncs.o array_selfuncs.o array_typanalyze.o \
jsonfuncs.o like.o lockfuncs.o mac.o misc.o nabstime.o name.o \
network.o network_gist.o network_selfuncs.o \
numeric.o numutils.o oid.o oracle_compat.o \
- orderedsetaggs.o pg_lzcompress.o pg_locale.o pg_lsn.o \
- pgstatfuncs.o pseudotypes.o quote.o rangetypes.o rangetypes_gist.o \
+ orderedsetaggs.o pg_locale.o pg_lsn.o pgstatfuncs.o \
+ pseudotypes.o quote.o rangetypes.o rangetypes_gist.o \
rangetypes_selfuncs.o rangetypes_spgist.o rangetypes_typanalyze.o \
regexp.o regproc.o ri_triggers.o rowtypes.o ruleutils.o \
selfuncs.o tid.o timestamp.o trigfuncs.o \
diff --git a/src/backend/utils/adt/pg_lzcompress.c b/src/backend/utils/adt/pg_lzcompress.c
deleted file mode 100644
index fe08890..000
--- a/src/backend/utils/adt/pg_lzcompress.c
+++ /dev/null
@@ -1,779 +0,0 @@
-/* --
- * pg_lzcompress.c -
- *
- * This is an implementation of LZ compression for PostgreSQL.
- * It uses a simple history table and generates 2-3 byte tags
- * capable of backward copy information for 3-273 bytes with
- * a max offset of 4095.
- *
- * Entry routines:
- *
- * bool
- * pglz_compress(const char *source, int32 slen, PGLZ_Header *dest,
- * const PGLZ_Strategy
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
On Wed, Nov 26, 2014 at 8:41 AM, Palle Girgensohn gir...@pingpong.net wrote: Hi! This is indeed a very well tested patch as we've run it in production for 8+ years on 20+ systems. It is not included upstreams mainly because I did ask for it to happen. I've been aiming to do it but haven't got around to it. Also, since 9.2 (?) there is support in PostgreSQL for setting collate locale per column. This is not yet supported by the patch, which makes it non-complete. You could argue that this is not as important as supporting the primary locale, but it would be hard to argue about that, it would have to be added for it to reach inclusion upstreams. So, I can vouch for it, it does the job just fine. Upstreams support will happen eventually. We did also discuss this back when we did the Windows port. One of the big arguments against bringing it in then (because it worked) was that we'd bring in another compile time dependency that's actually larger than PostgreSQL itself. For example,the ICU .tgz file of the latest version is 24.3Mb, and the latest postgresql .tgz is 21.8Mb. If we add it as a requirement, we more than double the size of PostgreSQL. (Part of that was specifically a concern on Windows of course, since no dependencies can be expected to exist there - icu is a lot more likely to already exist packaged up on linux/bsd) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
26 nov 2014 kl. 09:58 skrev Magnus Hagander mag...@hagander.net: On Wed, Nov 26, 2014 at 8:41 AM, Palle Girgensohn gir...@pingpong.net wrote: Hi! This is indeed a very well tested patch as we've run it in production for 8+ years on 20+ systems. It is not included upstreams mainly because I did ask for it to happen. I've been aiming to do it but haven't got around to it. Also, since 9.2 (?) there is support in PostgreSQL for setting collate locale per column. This is not yet supported by the patch, which makes it non-complete. You could argue that this is not as important as supporting the primary locale, but it would be hard to argue about that, it would have to be added for it to reach inclusion upstreams. So, I can vouch for it, it does the job just fine. Upstreams support will happen eventually. We did also discuss this back when we did the Windows port. One of the big arguments against bringing it in then (because it worked) was that we'd bring in another compile time dependency that's actually larger than PostgreSQL itself. For example,the ICU .tgz file of the latest version is 24.3Mb, and the latest postgresql .tgz is 21.8Mb. If we add it as a requirement, we more than double the size of PostgreSQL. (Part of that was specifically a concern on Windows of course, since no dependencies can be expected to exist there - icu is a lot more likely to already exist packaged up on linux/bsd) For windows, that is very good argument. ICU is huge and takes forever to build. But as you say, it is a lot more likely to already be installed or at least packaged. Also, you where, rightly, reluctant to use the ICU patch at that time because it required a memcopy (from utf-8 to ICUs internal utf-16) of every column it was to compare. This requirement is of course long gone, as ICU soon after fixed built in optimizations for utf-8, a very reasonable development step for the ICU platform... :-) Jakob, including the patch in PostgreSQL.app seems pretty reasonable. There's is only a small fraction of ICU that is used, a couple of libraries I believe. As I said, the missing feature will probably be fixed some time in the future, after which I will suggest the patch for inclusion. But it is not even near the top of my to-do list. :-/ Cheers, Palle -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PITR failing to stop before DROP DATABASE
Re: Heikki Linnakangas 2014-11-25 5474b848.3060...@vmware.com db1 is registered in pg_database, but the directory is missing on disk. Yeah, DROP DATABASE cheats. It deletes all the files first, and commits the transaction only after that. There's this comment at the end of dropdb() function: Oh ok. So this is an artifact of the non-transactionality (is this a word?) of CREATE DATABASE. /* * Force synchronous commit, thus minimizing the window between removal of * the database files and commital of the transaction. If we crash before * committing, we'll have a DB that's gone on disk but still there * according to pg_database, which is not good. */ So you could see the same after crash recovery, but it's a lot easier to reproduce with PITR. This could be fixed by doing DROP DATABASE the same way we do DROP TABLE. At the DROP DATABASE command, just memorize the OID of the dropped database, but don't delete anything yet. Perform the actual deletion after flushing the commit record to disk. But then you would have the opposite problem - you might be left with a database that's dropped according to pg_database, but the files are still present on disk. My concern is mostly that PITR to just before an accidental DROP DATABASE is one of the primary use cases for PITR, so it should Just Work. (I ran into this during a training and had a hard time explaining why PITR bugs exist :) I just did another test, and as expected, the problem goes away if I execute any transaction just before the DROP DATABASE - even a simple SELECT txid_current() is enough. So my suggestion for a simple fix would be to make DROP DATABASE execute a short fake transaction before it starts deleting files and then continue as before. This would serve as a stopping point for recovery_target_time to run into. (We could still fix this properly later, but this idea seems like a good fix for a practical problem that doesn't break anything else.) Christoph -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PITR failing to stop before DROP DATABASE
On 11/26/2014 11:19 AM, Christoph Berg wrote: Re: Heikki Linnakangas 2014-11-25 5474b848.3060...@vmware.com db1 is registered in pg_database, but the directory is missing on disk. Yeah, DROP DATABASE cheats. It deletes all the files first, and commits the transaction only after that. There's this comment at the end of dropdb() function: Oh ok. So this is an artifact of the non-transactionality (is this a word?) of CREATE DATABASE. DROP DATABASE. CREATE DATABASE is a different story. It does similar non-transactional tricks and has similar issues, but it's a completely different codepath and could be fixed independently of DROP DATABASE. /* * Force synchronous commit, thus minimizing the window between removal of * the database files and commital of the transaction. If we crash before * committing, we'll have a DB that's gone on disk but still there * according to pg_database, which is not good. */ So you could see the same after crash recovery, but it's a lot easier to reproduce with PITR. This could be fixed by doing DROP DATABASE the same way we do DROP TABLE. At the DROP DATABASE command, just memorize the OID of the dropped database, but don't delete anything yet. Perform the actual deletion after flushing the commit record to disk. But then you would have the opposite problem - you might be left with a database that's dropped according to pg_database, but the files are still present on disk. My concern is mostly that PITR to just before an accidental DROP DATABASE is one of the primary use cases for PITR, so it should Just Work. (I ran into this during a training and had a hard time explaining why PITR bugs exist :) I just did another test, and as expected, the problem goes away if I execute any transaction just before the DROP DATABASE - even a simple SELECT txid_current() is enough. So my suggestion for a simple fix would be to make DROP DATABASE execute a short fake transaction before it starts deleting files and then continue as before. This would serve as a stopping point for recovery_target_time to run into. (We could still fix this properly later, but this idea seems like a good fix for a practical problem that doesn't break anything else.) Yeah, seems reasonable. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
One of the big arguments against bringing it in then (because it worked) was that we'd bring in another compile time dependency that's actually larger than PostgreSQL itself. Magnus: I don't see how this is a problem as long as using ICU is *optional*. On systems with a working strcoll there is no problem with using the stdc functions (except that ICU might offer more collations). Jakob, including the patch in PostgreSQL.app seems pretty reasonable. There's is only a small fraction of ICU that is used, a couple of libraries I believe. Palle: The ICU libraries themselves aren't that big, but the required data files (also packaged as a dynamic library) are big (around 25MB uncompressed). However, I'd rather increase the download size by 30% than ship a broken database. Best regards, Jakob
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
26 nov 2014 kl. 10:36 skrev Jakob Egger ja...@eggerapps.at: One of the big arguments against bringing it in then (because it worked) was that we'd bring in another compile time dependency that's actually larger than PostgreSQL itself. Magnus: I don't see how this is a problem as long as using ICU is *optional*. On systems with a working strcoll there is no problem with using the stdc functions (except that ICU might offer more collations). In windows, it was primarily about packaging, I believe. Mind you, this was many years ago... ;) Jakob, including the patch in PostgreSQL.app seems pretty reasonable. There's is only a small fraction of ICU that is used, a couple of libraries I believe. Palle: The ICU libraries themselves aren't that big, but the required data files (also packaged as a dynamic library) are big (around 25MB uncompressed). However, I'd rather increase the download size by 30% than ship a broken database. Bear in mind that this might alter the way indexes are built. From the top of my head, I just can't remember if this is true or not. I'm probably wrong? Magnus? You would have to try. It does change the order by to properly handle utf-8 *AND* order by becomes case insensitve. I'm not sure this is correct SQL? I know that in Oracle, this is optional (NLS_COMP=LINGUISTIC and/or NLS_SORT=BINARY_CI), and SQL Server has something similar. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
Bear in mind that this might alter the way indexes are built. From the top of my head, I just can't remember if this is true or not. I'm probably wrong? Magnus? You would have to try. That's why I want to include it in the first version of 9.4, when people need to dump reload their database anyway (I'll make a note not to use pg_upgrade) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
26 nov 2014 kl. 10:48 skrev Jakob Egger ja...@eggerapps.at: Bear in mind that this might alter the way indexes are built. From the top of my head, I just can't remember if this is true or not. I'm probably wrong? Magnus? You would have to try. That's why I want to include it in the first version of 9.4, when people need to dump reload their database anyway (I'll make a note not to use pg_upgrade) Good point. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
On Wed, Nov 26, 2014 at 9:48 AM, Jakob Egger ja...@eggerapps.at wrote: Bear in mind that this might alter the way indexes are built. From the top of my head, I just can't remember if this is true or not. I'm probably wrong? Magnus? You would have to try. That's why I want to include it in the first version of 9.4, when people need to dump reload their database anyway (I'll make a note not to use pg_upgrade) You may want to bear in mind that postgres.app is on the main PG downloads page on the website. If you're patching Postgres to add a feature like this, it would become a fork and would have to be moved out of the PostgreSQL Core Distribution section of the download area as we only include pure distributions there. -- Dave Page PostgreSQL Core Team http://www.postgresql.org/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
Am 26.11.2014 um 11:05 schrieb Dave Page dp...@postgresql.org: You may want to bear in mind that postgres.app is on the main PG downloads page on the website. If you're patching Postgres to add a feature like this, it would become a fork and would have to be moved out of the PostgreSQL Core Distribution section of the download area as we only include pure distributions there. I wasn't aware of this. I'll have to bring this up on the Postgres.app Github page. Personally, I don't think that shipping a database with broken text sorting is acceptable; but I can't speak on behalf of the other contributors to Postgres.app without consulting them first.
[HACKERS] issue in postgresql 9.1.3 in using arrow key in Solaris platform
Hi all, We are facing following issue in postgresql 9.1.3 in using arrow key in Solaris platform. Can you please help us to resolve it or any new release has fix for this or any workaround for this? issue: psql client generates a core when up arrow is used twice. Platfrom: Solaris X86 Steps to reproduce: = 1. Login to any postgres database 2. execute any quer say \list 3. press up arrow twice. 4. segmentation fault occurs and core is generated. Also session is terminated. PLease find example below # ./psql -U super -d mgrdb Password for user super: psql (9.1.3) Type help for help. mgrdb=# \l List of databases Name| Owner | Encoding | Collate |Ctype| Access privileg es ---+--+--+-+-+-- - mgrdb | postgres | UTF8 | en_US.UTF-8 | en_US.UTF-8 | postgres | postgres | UTF8 | en_US.UTF-8 | en_US.UTF-8 | template0 | postgres | UTF8 | en_US.UTF-8 | en_US.UTF-8 | =c/postgres + | | | | | postgres=CTc/post gres template1 | postgres | UTF8 | en_US.UTF-8 | en_US.UTF-8 | =c/postgres + | | | | | postgres=CTc/post gres (4 rows) mgrdb=# mgrdb=# select count(1) from operator_msm;Segmentation Fault (core dumped) Regards Tarkeshwar
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
On Wed, Nov 26, 2014 at 10:13 AM, Jakob Egger ja...@eggerapps.at wrote: Am 26.11.2014 um 11:05 schrieb Dave Page dp...@postgresql.org: You may want to bear in mind that postgres.app is on the main PG downloads page on the website. If you're patching Postgres to add a feature like this, it would become a fork and would have to be moved out of the PostgreSQL Core Distribution section of the download area as we only include pure distributions there. I wasn't aware of this. I'll have to bring this up on the Postgres.app Github page. Personally, I don't think that shipping a database with broken text sorting is acceptable; but I can't speak on behalf of the other contributors to Postgres.app without consulting them first. Right - but the correct course of action would be to get the problem fixed in PostgreSQL itself, not to fork the code which could lead to other problems for users. -- Dave Page PostgreSQL Core Team http://www.postgresql.org/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
On Wed, Nov 26, 2014 at 11:13 AM, Jakob Egger ja...@eggerapps.at wrote: Am 26.11.2014 um 11:05 schrieb Dave Page dp...@postgresql.org: You may want to bear in mind that postgres.app is on the main PG downloads page on the website. If you're patching Postgres to add a feature like this, it would become a fork and would have to be moved out of the PostgreSQL Core Distribution section of the download area as we only include pure distributions there. I wasn't aware of this. I'll have to bring this up on the Postgres.app Github page. Personally, I don't think that shipping a database with broken text sorting is acceptable; but I can't speak on behalf of the other contributors to Postgres.app without consulting them first. Is it broken *worse* in 9.4 than it was in previous versions? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
Am 26.11.2014 um 11:20 schrieb Dave Page dp...@postgresql.org: On Wed, Nov 26, 2014 at 10:13 AM, Jakob Egger ja...@eggerapps.at wrote: Am 26.11.2014 um 11:05 schrieb Dave Page dp...@postgresql.org: You may want to bear in mind that postgres.app is on the main PG downloads page on the website. If you're patching Postgres to add a feature like this, it would become a fork and would have to be moved out of the PostgreSQL Core Distribution section of the download area as we only include pure distributions there. I wasn't aware of this. I'll have to bring this up on the Postgres.app Github page. Personally, I don't think that shipping a database with broken text sorting is acceptable; but I can't speak on behalf of the other contributors to Postgres.app without consulting them first. Right - but the correct course of action would be to get the problem fixed in PostgreSQL itself, not to fork the code which could lead to other problems for users. Agreed. Since this isn't a priority for Palle I'll have a look at the patch to see if I can extend it to make it suitable for submitting it, but since I have never contributed source to PostgreSQL I don't know yet if I can handle it. I've opened an issue on Github to discuss what to do about Postgres.app and the upcoming 9.4 release: https://github.com/PostgresApp/PostgresApp/issues/233 https://github.com/PostgresApp/PostgresApp/issues/233 Best regards, Jakob
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
Is it broken *worse* in 9.4 than it was in previous versions? No. Because the indices need to be rebuilt, the only realistic opportunity for applying this patch to Postgres.app is when releasing a major new version, since then people need to migrate their data anyway. That's why I wanted to apply the patch when 9.4 is released. I'm starting to see that maybe not all bugs can be fixed right now; I'm now waiting for input from the other contributors on Github. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes
Hello, I would like to contribute few points. XLogInsertRecord(XLogRecData *rdata, XLogRecPtr fpw_lsn) RedoRecPtr = Insert-RedoRecPtr; } doPageWrites = (Insert-fullPageWrites || Insert-forcePageWrites); doPageCompression = (Insert-fullPageWrites == FULL_PAGE_WRITES_COMPRESS); Don't we need to initialize doPageCompression similar to doPageWrites in InitXLOGAccess? Also , in the earlier patches compression was set 'on' even when fpw GUC is 'off'. This was to facilitate compression of FPW which are forcibly written even when fpw GUC is turned off. doPageCompression in this patch is set to true only if value of fpw GUC is 'compress'. I think its better to compress forcibly written full page writes. Regards, Rahila Syed -Original Message- From: pgsql-hackers-ow...@postgresql.org [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier Sent: Wednesday, November 26, 2014 1:55 PM To: Alvaro Herrera Cc: Andres Freund; Robert Haas; Fujii Masao; Rahila Syed; Rahila Syed; PostgreSQL-development Subject: Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes So, Here are reworked patches for the whole set, with the following changes: - Found why replay was failing, xlogreader.c took into account BLCKSZ - hole while it should have taken into account the compressed data length when fetching a compressed block image. - Reworked pglz portion to have it return status errors instead of simple booleans. pglz stuff is as well moved to src/common as Alvaro suggested. I am planning to run some tests to check how much compression can reduce WAL size with this new set of patches. I have been however able to check that those patches pass installcheck-world with a standby replaying the changes behind. Feel free to play with those patches... Regards, -- Michael __ Disclaimer: This email and any attachments are sent in strictest confidence for the sole use of the addressee and may contain legally privileged, confidential, and proprietary data. If you are not the intended recipient, please advise the sender by replying promptly to this email and then delete and destroy this email and any attachments without any further use, copying or forwarding. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Using RTLD_DEEPBIND to handle symbol conflicts in loaded libraries
I had to make oracle_fdw work with PostgreSQL compiled using --with-ldap. The issue there is that Oracle's client library has the delightful property of linking against a ldap library they bundle that has symbol conflicts with OpenLDAP. At PostgreSQL startup libldap is loaded, so when libclntsh.so (the Oracle client) is loaded it gets bound to OpenLDAP symbols, and unsurprisingly crashes with a segfault when those functions get used. glibc-2.3.4+ has a flag called RTLD_DEEPBIND for dlopen that prefers symbols loaded by the library to those provided by the caller. Using this flag fixes my issue, PostgreSQL gets the ldap functions from libldap, Oracle client gets them from whatever it links to. Both work fine. Attached is a patch that enables this flag on Linux when available. This specific case could also be fixed by rewriting oracle_fdw to use dlopen for libclntsh.so and pass this flag, but I think it would be better to enable it for all PostgreSQL loaded extension modules. I can't think of a sane use case where it would be correct to prefer PostgreSQL loaded symbols to those the library was actually linked against. Does anybody know of a case where this flag wouldn't be a good idea? Are there any similar options for other platforms? Alternatively, does anyone know of linker flags that would give a similar effect? Regards, Ants Aasma -- Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt Web: http://www.postgresql-support.de diff --git a/src/backend/port/dynloader/linux.h b/src/backend/port/dynloader/linux.h index db2ac66..34fd35c 100644 --- a/src/backend/port/dynloader/linux.h +++ b/src/backend/port/dynloader/linux.h @@ -25,8 +25,12 @@ /* * In some older systems, the RTLD_NOW flag isn't defined and the mode * argument to dlopen must always be 1. The RTLD_GLOBAL flag is wanted - * if available, but it doesn't exist everywhere. - * If it doesn't exist, set it to 0 so it has no effect. + * if available, but it doesn't exist everywhere. The RTLD_DEEPBIND flag + * may also be missing, but if it is available we want to enable it so + * extensions we load prefer symbols from libraries they were linked + * against to conflicting symbols from unrelated libraries loaded by + * PostgreSQL. + * If the optional flags don't exist, set them to 0 so they have no effect. */ #ifndef RTLD_NOW #define RTLD_NOW 1 @@ -34,8 +38,11 @@ #ifndef RTLD_GLOBAL #define RTLD_GLOBAL 0 #endif +#ifndef RTLD_DEEPBIND +#define RTLD_DEEPBIND 0 +#endif -#define pg_dlopen(f) dlopen((f), RTLD_NOW | RTLD_GLOBAL) +#define pg_dlopen(f) dlopen((f), RTLD_NOW | RTLD_GLOBAL | RTLD_DEEPBIND) #define pg_dlsym dlsym #define pg_dlclose dlclose #define pg_dlerror dlerror -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PITR failing to stop before DROP DATABASE
Re: Heikki Linnakangas 2014-11-26 54759bc0.4070...@vmware.com Oh ok. So this is an artifact of the non-transactionality (is this a word?) of CREATE DATABASE. DROP DATABASE. CREATE DATABASE is a different story. It does similar non-transactional tricks and has similar issues, but it's a completely different codepath and could be fixed independently of DROP DATABASE. Err right. Too early in the morning... So my suggestion for a simple fix would be to make DROP DATABASE execute a short fake transaction before it starts deleting files and then continue as before. This would serve as a stopping point for recovery_target_time to run into. (We could still fix this properly later, but this idea seems like a good fix for a practical problem that doesn't break anything else.) Yeah, seems reasonable. Here's a first shot at a patch. It's not working yet because I think the commit isn't doing anything because no work was done in the transaction yet. *** a/src/backend/commands/dbcommands.c --- b/src/backend/commands/dbcommands.c *** dropdb(const char *dbname, bool missing_ *** 778,783 --- 778,798 nslots_active; /* +* Commit now to cause a commit xlog record to be logged. (We are outside +* any transaction so this is safe to do.) If we don't do this here, doing +* a PITR restore to just before DROP DATABASE will cause the files on disk +* to be deleted, while PITR stops before removing the database from the +* system catalogs, so the database is still visible while it is in fact +* already deleted. It is still possible to get to this intermediate state +* by selecting the correct transaction number in recovery.conf, but this +* fixes the common use case of specifying a recovery target time just +* before DROP DATABASE. +*/ + PopActiveSnapshot(); + CommitTransactionCommand(); + StartTransactionCommand(); + + /* * Look up the target database's OID, and get exclusive lock on it. We * need this to ensure that no new backend starts up in the target * database while we are deleting it (see postinit.c), and that no one is Christoph -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
2014-11-26 9:58 GMT+01:00 Magnus Hagander mag...@hagander.net: On Wed, Nov 26, 2014 at 8:41 AM, Palle Girgensohn gir...@pingpong.net wrote: Hi! This is indeed a very well tested patch as we've run it in production for 8+ years on 20+ systems. It is not included upstreams mainly because I did ask for it to happen. I've been aiming to do it but haven't got around to it. Also, since 9.2 (?) there is support in PostgreSQL for setting collate locale per column. This is not yet supported by the patch, which makes it non-complete. You could argue that this is not as important as supporting the primary locale, but it would be hard to argue about that, it would have to be added for it to reach inclusion upstreams. So, I can vouch for it, it does the job just fine. Upstreams support will happen eventually. We did also discuss this back when we did the Windows port. One of the big arguments against bringing it in then (because it worked) was that we'd bring in another compile time dependency that's actually larger than PostgreSQL itself. For example,the ICU .tgz file of the latest version is 24.3Mb, and the latest postgresql .tgz is 21.8Mb. If we add it as a requirement, we more than double the size of PostgreSQL. (Part of that was specifically a concern on Windows of course, since no dependencies can be expected to exist there - icu is a lot more likely to already exist packaged up on linux/bsd) 24MB is not problem for mostly Windows users. I don't propose ICU as main solution for us, but it can be good alternative for some companies, that should to fix inconsistency in collation implementation between Windows and Linux. Czech collation in Windows and Linux can produces different results in some corner cases. Regards Pavel -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: plpgsql - Assert statement
On 11/26/14 8:55 AM, Pavel Stehule wrote: * should be assertions globally enabled/disabled? - I have no personal preference in this question. I think so. The way I would use this function is to put expensive checks into strategic locations which would only run when developing locally (and additionally perhaps in one of the test environments.) And in that case I'd like to globally disable them for the live environment. * can be ASSERT exception handled ? - I prefer this be unhandled exception - like query_canceled because It should not be masked by plpgsql EXCEPTION WHEN OTHERS ... I don't care much either way, as long as we get good information about what went wrong. A stack trace and hopefully something like print_strict_params for parameters to the expr. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Using RTLD_DEEPBIND to handle symbol conflicts in loaded libraries
Ants Aasma wrote: I had to make oracle_fdw work with PostgreSQL compiled using --with-ldap. The issue there is that Oracle's client library has the delightful property of linking against a ldap library they bundle that has symbol conflicts with OpenLDAP. At PostgreSQL startup libldap is loaded, so when libclntsh.so (the Oracle client) is loaded it gets bound to OpenLDAP symbols, and unsurprisingly crashes with a segfault when those functions get used. glibc-2.3.4+ has a flag called RTLD_DEEPBIND for dlopen that prefers symbols loaded by the library to those provided by the caller. Using this flag fixes my issue, PostgreSQL gets the ldap functions from libldap, Oracle client gets them from whatever it links to. Both work fine. I am aware of the problem, but this solution is new to me. My workaround so far has been to load OpenLDAP with the LD_PRELOAD environment variable on PostgreSQL start. But then you get a crash when Oracle uses LDAP functionality (directory naming). Attached is a patch that enables this flag on Linux when available. This specific case could also be fixed by rewriting oracle_fdw to use dlopen for libclntsh.so and pass this flag, but I think it would be better to enable it for all PostgreSQL loaded extension modules. I'll consider changing oracle_fdw in that fashion, even if that will only remedy the problem on Linux. I think that this patch is a good idea though. Yours, Laurenz Albe -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Adam, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I am simply breaking this out into its own thread from the discussion on additional role attributes ( http://www.postgresql.org/message-id/20141015052259.gg28...@tamriel.snowman.net ). Makes sense to me, thanks. Based on these above I have attached an initial WIP patch for review and discussion that takes a swing at changing the catalog representation. Just a quick initial look, but I don't think we want to #include parsenodes.h into pg_authid.h. Why not put the #define ROLE_ATTR_* into pg_authid.h instead? We have similar #define's in other catalog .h's (PROARGMODE_*, RELKIND_*, etc). I'm also not a huge fan of the hard-coded 255 for the default superuser. That goes back to the other question of if we should bother having those explicitly listed at all, but I'd suggest having a #define for 'all' bits to be true (for currently used bits) and then a comment above the superuser which references that #define (we can't use the #define directly or we'd be including pg_authid.h into pg_proc.h, which isn't a good idea either; if we really want to use the #define, genbki.pl could be adjusted to find the #define similar to what it does for PGUID and PGNSP). Thanks! Stephen signature.asc Description: Digital signature
[HACKERS] Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3
Tom, First of all, thanks for your help on IRC last time with that CREATE INDEX memory consumption problem. As has been pointed out in a stackexchange answer to my question[1], it is indeed the limitation of pre-9.4 versions, but the limit is imposed on memtuples array, rather than total memory the sort in CREATE INDEX may allocate. The memtuples won't grow further than MaxAllocSize and I've got 24x50x10^6 = 1200MB, which just doesn't fit. We've got a customer who is testing a migration to PostgreSQL-9.3 (from $some_other_db), thus they load the tables first (some of their tables have 10-100 million rows), then create the indexes and they constantly see disk sort being used despite lots of available RAM and maintenance_work_mem set to increasingly higher values. Now my question, is it feasible to back-patch this to 9.3? Or should we tell the customer to wait before 9.4 is released? Thanks. -- Alex [1] http://dba.stackexchange.com/questions/83600/postgresql-create-index-memory-requirement -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] superuser() shortcuts
On Sun, Nov 23, 2014 at 3:38 PM, Andres Freund and...@2ndquadrant.com wrote: I'm not really particular about which way we go with the specific wording (suggestions welcome..) but the inconsistency should be dealt with. Meh. +1 for meh. I don't mind making things consistent if it can be done while maintaining or improving the absolute quality of those error messages -- but if the changes involve a loss of detail, or moving information that used to be in the main error message into the detail, then I don't think it's worth it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.2 recovery/startup problems
Jeff Janes wrote: This is what I get in the log from the attempted restart: PST LOG: database system was interrupted; last known up at 2014-11-25 15:40:33 PST PST LOG: database system was not properly shut down; automatic recovery in progress PST LOG: redo starts at 84/EF80 PST LOG: record with zero length at 84/EF09AE18 PST LOG: redo done at 84/EF09AD28 PST LOG: last completed transaction was at log time 2014-11-25 15:42:09.173599-08 PST LOG: checkpoint starting: end-of-recovery immediate PST LOG: checkpoint complete: wrote 103 buffers (0.2%); 0 transaction log file(s) added, 246 removed, 7 recycled; write=0.002 s, sync=0.020 s, total=0.526 s; sync files=51, longest=0.003 s, average=0.000 s PST FATAL: could not create file base/16416/59288: File exists Maybe fire up pg_xlogdump to see what xlog record references that relfilenode. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
26 nov 2014 kl. 11:44 skrev Jakob Egger ja...@eggerapps.at: Am 26.11.2014 um 11:20 schrieb Dave Page dp...@postgresql.org: On Wed, Nov 26, 2014 at 10:13 AM, Jakob Egger ja...@eggerapps.at wrote: Am 26.11.2014 um 11:05 schrieb Dave Page dp...@postgresql.org: You may want to bear in mind that postgres.app is on the main PG downloads page on the website. If you're patching Postgres to add a feature like this, it would become a fork and would have to be moved out of the PostgreSQL Core Distribution section of the download area as we only include pure distributions there. I wasn't aware of this. I'll have to bring this up on the Postgres.app Github page. Personally, I don't think that shipping a database with broken text sorting is acceptable; but I can't speak on behalf of the other contributors to Postgres.app without consulting them first. Right - but the correct course of action would be to get the problem fixed in PostgreSQL itself, not to fork the code which could lead to other problems for users. Agreed. Since this isn't a priority for Palle I'll have a look at the patch to see if I can extend it to make it suitable for submitting it, but since I have never contributed source to PostgreSQL I don't know yet if I can handle it. Well, this discussion actually pushes the priority quite a bit for me -- someone else actually beeing interested about the patch... I thought it was just me... :)= Just for reference, the Linux collation is actaully also broken wrt to utf-8. It is better than others, but not correct. And lower()/upper() has many rather common cases where it is not working with wide characters. For example, the towupper only looks at one character at the time, but proper handling needs to look at adjacent characters in some languages. Either way, getting it into core would not happen before 9.5 anyway. I've opened an issue on Github to discuss what to do about Postgres.app and the upcoming 9.4 release: https://github.com/PostgresApp/PostgresApp/issues/233 Best regards, Jakob -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] superuser() shortcuts
* Robert Haas (robertmh...@gmail.com) wrote: On Sun, Nov 23, 2014 at 3:38 PM, Andres Freund and...@2ndquadrant.com wrote: I'm not really particular about which way we go with the specific wording (suggestions welcome..) but the inconsistency should be dealt with. Meh. +1 for meh. I don't mind making things consistent if it can be done while maintaining or improving the absolute quality of those error messages -- but if the changes involve a loss of detail, or moving information that used to be in the main error message into the detail, then I don't think it's worth it. Doesn't that argument then apply to the other messages which I pointed out in my follow-up to Andres, where the detailed info is in the hint and the main error message is essentially 'permission denied'? Also, if we're going to make these error-messages related to role attributes be 'you need role attribute X', should we consider doing the same for the regular 'permission denied' error messages? I can understand the arguments about loss of detail or having the detail in the hint instead of the error message. I don't understand why we'd want the messaging to be inconsistent. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3
Alex, * Alex Shulgin (a...@commandprompt.com) wrote: Tom, First of all, thanks for your help on IRC last time with that CREATE INDEX memory consumption problem. Doubt it was Tom, but if it was, wanna share what channel on IRC it was? :D Now my question, is it feasible to back-patch this to 9.3? Or should we tell the customer to wait before 9.4 is released? I'm aware of a few folks who have back-patched this change and use custom-built binaries, but it won't be done by the community/PGDG as it's a new feature and not a bug fix. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] superuser() shortcuts
On 2014-11-26 08:33:10 -0500, Stephen Frost wrote: * Robert Haas (robertmh...@gmail.com) wrote: On Sun, Nov 23, 2014 at 3:38 PM, Andres Freund and...@2ndquadrant.com wrote: I'm not really particular about which way we go with the specific wording (suggestions welcome..) but the inconsistency should be dealt with. Meh. +1 for meh. I don't mind making things consistent if it can be done while maintaining or improving the absolute quality of those error messages -- but if the changes involve a loss of detail, or moving information that used to be in the main error message into the detail, then I don't think it's worth it. Doesn't that argument then apply to the other messages which I pointed out in my follow-up to Andres, where the detailed info is in the hint and the main error message is essentially 'permission denied'? A permission error on a relation is easier to understand than one for some abstract 'replication' permission. Also, if we're going to make these error-messages related to role attributes be 'you need role attribute X', should we consider doing the same for the regular 'permission denied' error messages? I can understand the arguments about loss of detail or having the detail in the hint instead of the error message. I don't understand why we'd want the messaging to be inconsistent. I'm not generally against making them more consistent. I'm vehemently against making them consistent by making them worse. And the suggested changes are worse. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
I find it hard to believe the original premise of this thread. We knew there were some problems with OSX and FreeBSD but surely they can't be completely broken? What happens if you run ls with your locale set to something like fr_FR.UTF8 ? Does Apple not sell Macs in countries other than the US? There were a number of problems with using ICU including the large dependency and the limitations of the iterator model but the main issue was that it's fundamentally a choice between being consistent with every other application on your system and being consistent with other Postgres databases running on other OSes. Most people run multiple applications on one OS, not many databases on many OSes on their own with no other applications. If Postgres used ICU then its output would be inconsistent with things like sort or ls or your application programming language's comparison operators. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test programs in contrib
Here's a patch. This creates a new subdir src/test/modules and places the five initially proposed modules in there. They continue to have their makefile with the same ifdef USE_PGXS pattern; they are no longer installed by default. Because many of them had either test in their names or some other now-useless particle, I renamed them: worker_spi - bgworker test_decoding - logical_decoding dummy_seclabel - seclabel test_shm_mq - shm_mq test_parser - tsparser The renaming is not complete: the extensions continue to have the old names, for instance. If the consensus is to rename them completely I can finish that, or we can decide to keep the original names, but they all seem inappropriate to me. I haven't done anything about documentation. I thought a new chapter after Additional Supplied Modules, perhaps entitled Additional Sample Modules would be appropriate. I tweaked make targets check, installcheck, installcheck-world, check-world: they all run the additional tests now. For buildfarm, the client code will need to be updated to have a new stage for src/test/modules running make check. I haven't touched MSVC yet. Opinions on this approach please? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test programs in contrib
On Wed, Nov 26, 2014 at 9:27 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Here's a patch. This creates a new subdir src/test/modules and places the five initially proposed modules in there. They continue to have their makefile with the same ifdef USE_PGXS pattern; they are no longer installed by default. Because many of them had either test in their names or some other now-useless particle, I renamed them: worker_spi - bgworker test_decoding - logical_decoding dummy_seclabel - seclabel test_shm_mq - shm_mq test_parser - tsparser The renaming is not complete: the extensions continue to have the old names, for instance. If the consensus is to rename them completely I can finish that, or we can decide to keep the original names, but they all seem inappropriate to me. I haven't done anything about documentation. I thought a new chapter after Additional Supplied Modules, perhaps entitled Additional Sample Modules would be appropriate. I tweaked make targets check, installcheck, installcheck-world, check-world: they all run the additional tests now. For buildfarm, the client code will need to be updated to have a new stage for src/test/modules running make check. I haven't touched MSVC yet. Opinions on this approach please? I like the move. I dislike the renaming. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test programs in contrib
On Wed, Nov 26, 2014 at 12:27 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Here's a patch. This creates a new subdir src/test/modules and places the five initially proposed modules in there. They continue to have their makefile with the same ifdef USE_PGXS pattern; they are no longer installed by default. Because many of them had either test in their names or some other now-useless particle, I renamed them: worker_spi - bgworker test_decoding - logical_decoding dummy_seclabel - seclabel test_shm_mq - shm_mq test_parser - tsparser The renaming is not complete: the extensions continue to have the old names, for instance. If the consensus is to rename them completely I can finish that, or we can decide to keep the original names, but they all seem inappropriate to me. I haven't done anything about documentation. I thought a new chapter after Additional Supplied Modules, perhaps entitled Additional Sample Modules would be appropriate. I tweaked make targets check, installcheck, installcheck-world, check-world: they all run the additional tests now. For buildfarm, the client code will need to be updated to have a new stage for src/test/modules running make check. I haven't touched MSVC yet. Opinions on this approach please? The patch is missing... Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] no test programs in contrib
This is pretty bulky, but really the vast majority of the changes here are just git mv. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services test_modules.patch.gz Description: application/gzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [GENERAL] issue in postgresql 9.1.3 in using arrow key in Solaris platform
On 11/26/2014 02:16 AM, M Tarkeshwar Rao wrote: Hi all, We are facing following issue in postgresql 9.1.3 in using arrow key in Solaris platform. *Can you please help us to resolve it or any new release has fix for this or any workaround for this?* Would seem to me to be an interaction between Postgres and readline. Not sure exactly what, but some information would be helpful for those that might know: 1) What version of Solaris? 2) How was Postgres installed and from what source? 3) What version of readline is installed? 4) Are you using a psql client that is the same version as the server? issue: psql client generates a core when up arrow is used twice. Regards Tarkeshwar -- Adrian Klaver adrian.kla...@aklaver.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] superuser() shortcuts
* Andres Freund (and...@2ndquadrant.com) wrote: On 2014-11-26 08:33:10 -0500, Stephen Frost wrote: Doesn't that argument then apply to the other messages which I pointed out in my follow-up to Andres, where the detailed info is in the hint and the main error message is essentially 'permission denied'? A permission error on a relation is easier to understand than one for some abstract 'replication' permission. The more I consider this and review the error message reporting policy, the more I feel that the original coding was wrong and that this change *is* the correct one to make. Even the example in the documentation makes a point of having the errcode() and the errmsg() match up: ereport(ERROR, (errcode(ERRCODE_DIVISION_BY_ZERO), errmsg(division by zero))); The second example is similar: ereport(ERROR, (errcode(ERRCODE_AMBIGUOUS_FUNCTION), errmsg(function %s is not unique, func_signature_string(funcname, nargs, NIL, actual_arg_types)), errhint(Unable to choose a best candidate function. You might need to add explicit typecasts.))); Further, the comments around many of the places which use ACLCHECK_NOT_OWNER (which also uses the 'must be/have X' style) are permissions check for X, and what's more, we've had things go from one to the other previously even though the user action wasn't changing- specifically TRUNCATE used to be owner-only and was changed to be grantable. The reason for the error *is* permission denied, hence the errcode(). The detail is that the permission is reserved to the owner, or to roles which have a given attribute. The error isn't must by X. I'm of the opinion at this point that we should change the ACLCHECK_NOT_OWNER branch under aclcheck_error() to match what is proposed by this patch- have a 'permission denied' error for whatever the action is and then have errdetail of 'not_owner_msg[objectkind]'. I don't think we need to include errdetail() for the ACLCHECK_NO_PRIV case as those permissions are part of the normal GRANT/REVOKE privilege system. The detail is warranted when we're talking about things which are outside of the normal privilege system. If it isn't a permission denied error then it shouldn't be using ERRCODE_INSUFFICIENT_PRIVILEGE. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3
Stephen Frost sfr...@snowman.net writes: * Alex Shulgin (a...@commandprompt.com) wrote: Tom, First of all, thanks for your help on IRC last time with that CREATE INDEX memory consumption problem. Doubt it was Tom, but if it was, wanna share what channel on IRC it was? :D Must've been my evil twin. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] superuser() shortcuts
Stephen Frost wrote: * Andres Freund (and...@2ndquadrant.com) wrote: On 2014-11-26 08:33:10 -0500, Stephen Frost wrote: Doesn't that argument then apply to the other messages which I pointed out in my follow-up to Andres, where the detailed info is in the hint and the main error message is essentially 'permission denied'? A permission error on a relation is easier to understand than one for some abstract 'replication' permission. The more I consider this and review the error message reporting policy, the more I feel that the original coding was wrong and that this change *is* the correct one to make. +1. I don't care for the idea of not moving from main error message to errdetail -- the rationale seems to be that errdetail might be hidden, lost, or otherwise not read by the user; if that is so, why do we have errdetail in the first place? We might as well just move all the errdetails into the main message, huh? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
On Nov 26, 2014, at 8:21 AM, Greg Stark st...@mit.edu wrote: I find it hard to believe the original premise of this thread. We knew there were some problems with OSX and FreeBSD but surely they can't be completely broken? Ever tried to use Spotlight for searching (English) on the Mac, not completely broken, just not reliable. This does not surprise me in the least for OSX. The Mac has, in recent history, become a “looks good, but the details may or may not be really correct platform. I thought FreeBSD was a preferred OS for PostgreSQL? This does surprise me. What happens if you run ls with your locale set to something like fr_FR.UTF8 ? Does Apple not sell Macs in countries other than the US? Neil Daily Mac user for a long time. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] explain sortorder
Hello everyone, For creating indexes on more than one column, it is useful to know the sort order of each sort key. So now, if you run EXPLAIN in VERBOSE mode, you get the sort order information in the order the sort keys are displayed - Lukas - This patch is meant for discussion - It’s against the master branch - The patch compiles successfully and one test (inherit) is affected - There are no platform-specific items in this patch - The patch, as described, enhances EXPLAIN VERBOSE. For an example, see the regression test - There is no TODO item referring to this patch @patchname: explain_sortorder v2 @version: 2.01 @author: Marius Timmer mtimm...@uni-muenster.de, Arne Scheffer arne.schef...@uni-muenster.de, Lukas Kreft lukaskr...@uni-muenster.de @description: Display sort order options in VERBOSE mode of EXPLAIN - The situation Currently I am able to run a EXPLAIN-Statement (verbose) for getting more Information about a Query. But it is not enough to check in which order the results will be sorted, what could be interesting to modify some Statements so they can become more performant. - What this patch does This patch will add one more information to the result of an EXPLAIN- Statement in verbose-mode. You will find the new property Sort order which tells you the order of the used keys while sorting. You can use it in all available Formats. Greetings, Marius Timmer explain_sortorder-v2.patch Description: Binary data --- Marius Timmer Zentrum für Informationsverarbeitung Westfälische Wilhelms-Universität Münster Einsteinstraße 60 mtimm...@uni-muenster.de smime.p7s Description: S/MIME cryptographic signature
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
26 nov 2014 kl. 15:21 skrev Greg Stark st...@mit.edu:
I find it hard to believe the original premise of this thread. We knew
there were some problems with OSX and FreeBSD but surely they can't be
completely broken? What happens if you run ls with your locale set
to something like fr_FR.UTF8 ? Does Apple not sell Macs in countries
other than the US?
Hi,
On Mac OS X, ls -l is completely broken wrt utf-8 collation. Really. Horribly
broken. The sorting it produces for the Swedish locale is just nonexisting,
completely unaccetable, unusable. Compare it to sorting Z just after S or
something, just to get the scale of how bad it is.
Application languages like Java have their own sorting. C based stuff like perl
have their own way to do it. python, well depends on the version, haven't
checked. C applications, well, it depends on if they use ICU or not, I guess. :)
Apples sells computers, but does not really promote using locales in
Terminal.app... :)=
There were a number of problems with using ICU including the large
dependency and the limitations of the iterator model but the main
issue was that it's fundamentally a choice between being consistent
with every other application on your system and being consistent with
other Postgres databases running on other OSes. Most people run
multiple applications on one OS, not many databases on many OSes on
their own with no other applications. If Postgres used ICU then its
output would be inconsistent with things like sort or ls or your
application programming language's comparison operators.
I think most people don't care about getting postgresql collation consistent
with sort or ls, they just want it to work properly for real life applications,
so users who really don't care about ls or sort get the result they expect. Or,
they give up and sort it in the application instead (=fail). But I guess that
depends on which applications you use. We've used the patch for 8+ years. For
us, Linux built-in collation would not have been enough either -- if memory
serves it fails to sort 'ß' together with 'ss', and also fails to upper('ß') =
'SS', which would be expected in the real world.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] superuser() shortcuts
* Tom Lane (t...@sss.pgh.pa.us) wrote: In the context at hand, I think most of the messages in question are currently phrased like must be superuser to do X. I'd be fine with changing that to permission denied to do X, but not to just permission denied. Apologies for the terseness of my (earlier) reply. This is exactly what I'm suggesting. What was in the patch was this change: ! ERROR: must be superuser or replication role to use replication slots --- ! ERROR: permission denied to use replication slots ! HINT: You must be superuser or replication role to use replication slots. On reflection, perhaps it should be an errdetail() message instead of an errhint() message, but the complaint levied against the change was making it be 'permission denied to X' and moving the 'must be superuser' out of the errmsg(). Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] superuser() shortcuts
On 2014-11-26 11:53:40 -0300, Alvaro Herrera wrote: Stephen Frost wrote: * Andres Freund (and...@2ndquadrant.com) wrote: On 2014-11-26 08:33:10 -0500, Stephen Frost wrote: Doesn't that argument then apply to the other messages which I pointed out in my follow-up to Andres, where the detailed info is in the hint and the main error message is essentially 'permission denied'? A permission error on a relation is easier to understand than one for some abstract 'replication' permission. The more I consider this and review the error message reporting policy, the more I feel that the original coding was wrong and that this change *is* the correct one to make. +1. I don't care for the idea of not moving from main error message to errdetail -- the rationale seems to be that errdetail might be hidden, lost, or otherwise not read by the user; if that is so, why do we have errdetail in the first place? We might as well just move all the errdetails into the main message, huh? That rationale is imo bogus. The actual error message should be helpful and informative - only if the error would be to verbose it should go into the detail. It's not exactly unimportant to know why something was prohibited. It's not like this is any form of new precedent - *loads* of errmsg() style of messages have more information than the static string. I don't see how you read the contrary from the guidelines: The primary message should be short, factual, and avoid reference to implementation details such as specific function names. Short means should fit on one line under normal conditions. Use a detail message if needed to keep the primary message short, or if you feel a need to mention implementation details such as the particular system call that failed. Both primary and detail messages should be factual. Use a hint message for suggestions about what to do to fix the problem, especially if the suggestion might not always be applicable. It's quite common that you'll only see the actual error message in logs and displayed error messages. That seems to give credence to *my* position, not yours. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
26 nov 2014 kl. 15:56 skrev Neil Tiffin ne...@neiltiffin.com: On Nov 26, 2014, at 8:21 AM, Greg Stark st...@mit.edu wrote: I find it hard to believe the original premise of this thread. We knew there were some problems with OSX and FreeBSD but surely they can't be completely broken? Ever tried to use Spotlight for searching (English) on the Mac, not completely broken, just not reliable. This does not surprise me in the least for OSX. The Mac has, in recent history, become a “looks good, but the details may or may not be really correct platform. I thought FreeBSD was a preferred OS for PostgreSQL? This does surprise me. It works fine if you use the English language, or if you don't use utf-8. And it works fine with utf-8 if you don't care about real world sorting, or if you do the sorting in your application anyway (most OS:es collations are really broken for non-english locales anyway). So for a great number of people, it works great. For the rest of us, well, I use ICU... :) What happens if you run ls with your locale set to something like fr_FR.UTF8 ? Does Apple not sell Macs in countries other than the US? Neil Daily Mac user for a long time. -- Sent via pgsql-packagers mailing list (pgsql-packag...@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-packagers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test programs in contrib
On 2014-11-26 10:08:57 -0500, Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: This is pretty bulky, but really the vast majority of the changes here are just git mv. For ease of review, is there a way to get git to show just the diffs that *aren't* git mv? (That is, show changes in a file's content without respect to its having moved?) Yes, that's possible. git diff/show/whatever -M Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
26 nov 2014 kl. 14:06 skrev Palle Girgensohn gir...@pingpong.net: Well, this discussion actually pushes the priority quite a bit for me -- someone else actually beeing interested about the patch... I thought it was just me... :)= By pushes the priority, I mean it gets more prioritized, in case that was unclear. :) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] superuser() shortcuts
* Andres Freund (and...@2ndquadrant.com) wrote: I don't see how you read the contrary from the guidelines: The primary message should be short, factual, and avoid reference to implementation details such as specific function names. Short means should fit on one line under normal conditions. Use a detail message if needed to keep the primary message short, or if you feel a need to mention implementation details such as the particular system call that failed. Both primary and detail messages should be factual. Use a hint message for suggestions about what to do to fix the problem, especially if the suggestion might not always be applicable. It's quite common that you'll only see the actual error message in logs and displayed error messages. Having to be the owner of a relation to TRUNCATE it was an implementation detail, and one which changed back in 2008. Needing to have the replication role attribute is an implementation detail. It's not the error- the error is *permission denied to do X*. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] no test programs in contrib
Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: This is pretty bulky, but really the vast majority of the changes here are just git mv. For ease of review, is there a way to get git to show just the diffs that *aren't* git mv? (That is, show changes in a file's content without respect to its having moved?) I think git diff -D -M -B does that. Here's such a diff, which I obtained from git show (otherwise you'd need to git add all the new files, I think, which would be pretty annoying) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services commit 3528d3323dd6cf086c9326432293827cd003c783 Author: Alvaro Herrera alvhe...@alvh.no-ip.org Date: Wed Nov 26 10:16:54 2014 -0300 src/test/modules diff --git a/GNUmakefile.in b/GNUmakefile.in index 69e0824..3a57495 100644 --- a/GNUmakefile.in +++ b/GNUmakefile.in @@ -65,6 +65,7 @@ check check-tests: all check check-tests installcheck installcheck-parallel installcheck-tests: $(MAKE) -C src/test/regress $@ + $(MAKE) -C src/test/modules $@ $(call recurse,check-world,src/test src/pl src/interfaces/ecpg contrib src/bin,check) diff --git a/contrib/Makefile b/contrib/Makefile index b37d0dd..efee109 100644 --- a/contrib/Makefile +++ b/contrib/Makefile @@ -16,7 +16,6 @@ SUBDIRS = \ dblink \ dict_int \ dict_xsyn \ - dummy_seclabel \ earthdistance \ file_fdw \ fuzzystrmatch \ @@ -50,13 +49,9 @@ SUBDIRS = \ spi \ tablefunc \ tcn \ - test_decoding \ - test_parser \ - test_shm_mq \ tsearch2 \ unaccent \ - vacuumlo \ - worker_spi + vacuumlo ifeq ($(with_openssl),yes) SUBDIRS += sslinfo diff --git a/contrib/pg_upgrade/test.sh b/contrib/pg_upgrade/test.sh index 7bbd2c7..dcc84f3 100644 --- a/contrib/pg_upgrade/test.sh +++ b/contrib/pg_upgrade/test.sh @@ -65,6 +65,7 @@ if [ $1 = '--install' ]; then $MAKE -s -C ../.. install DESTDIR=$temp_install $MAKE -s -C ../pg_upgrade_support install DESTDIR=$temp_install $MAKE -s -C . install DESTDIR=$temp_install + $MAKE -s -C ../../src/test/modules install DESTDIR=$temp_install # platform-specific magic to find the shared libraries; see pg_regress.c LD_LIBRARY_PATH=$libdir:$LD_LIBRARY_PATH diff --git a/src/test/Makefile b/src/test/Makefile index 0fd7eab..5d997b8 100644 --- a/src/test/Makefile +++ b/src/test/Makefile @@ -12,6 +12,6 @@ subdir = src/test top_builddir = ../.. include $(top_builddir)/src/Makefile.global -SUBDIRS = regress isolation +SUBDIRS = regress isolation modules $(recurse) diff --git a/contrib/worker_spi/Makefile b/src/test/modules/bgworker/Makefile similarity index 76% rename from contrib/worker_spi/Makefile rename to src/test/modules/bgworker/Makefile index 5cce4d1..673281a 100644 --- a/contrib/worker_spi/Makefile +++ b/src/test/modules/bgworker/Makefile @@ -1,4 +1,4 @@ -# contrib/worker_spi/Makefile +# src/test/modules/bgworker/Makefile MODULES = worker_spi @@ -11,8 +11,8 @@ PG_CONFIG = pg_config PGXS := $(shell $(PG_CONFIG) --pgxs) include $(PGXS) else -subdir = contrib/worker_spi -top_builddir = ../.. +subdir = src/test/modules/bgworker +top_builddir = ../../../.. include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk endif diff --git a/contrib/worker_spi/worker_spi--1.0.sql b/src/test/modules/bgworker/worker_spi--1.0.sql similarity index 100% rename from contrib/worker_spi/worker_spi--1.0.sql rename to src/test/modules/bgworker/worker_spi--1.0.sql diff --git a/contrib/worker_spi/worker_spi.c b/src/test/modules/bgworker/worker_spi.c similarity index 100% rename from contrib/worker_spi/worker_spi.c rename to src/test/modules/bgworker/worker_spi.c diff --git a/contrib/worker_spi/worker_spi.control b/src/test/modules/bgworker/worker_spi.control similarity index 100% rename from contrib/worker_spi/worker_spi.control rename to src/test/modules/bgworker/worker_spi.control diff --git a/contrib/test_decoding/.gitignore b/src/test/modules/logical_decoding/.gitignore similarity index 100% rename from contrib/test_decoding/.gitignore rename to src/test/modules/logical_decoding/.gitignore diff --git a/contrib/test_decoding/Makefile b/src/test/modules/logical_decoding/Makefile similarity index 78% rename from contrib/test_decoding/Makefile rename to src/test/modules/logical_decoding/Makefile index 438be44..0bc85d8 100644 --- a/contrib/test_decoding/Makefile +++ b/src/test/modules/logical_decoding/Makefile @@ -1,4 +1,4 @@ -# contrib/test_decoding/Makefile +# src/test/modules/logical_decoding/Makefile MODULES = test_decoding PGFILEDESC = test_decoding - example of a logical decoding output plugin @@ -12,8 +12,8 @@ PG_CONFIG = pg_config PGXS := $(shell $(PG_CONFIG) --pgxs) include $(PGXS) else -subdir = contrib/test_decoding -top_builddir = ../.. +subdir = src/test/modules/logical_decoding +top_builddir = ../../../.. include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk
Re: [HACKERS] superuser() shortcuts
On 2014-11-26 10:18:20 -0500, Stephen Frost wrote: * Andres Freund (and...@2ndquadrant.com) wrote: I don't see how you read the contrary from the guidelines: The primary message should be short, factual, and avoid reference to implementation details such as specific function names. Short means should fit on one line under normal conditions. Use a detail message if needed to keep the primary message short, or if you feel a need to mention implementation details such as the particular system call that failed. Both primary and detail messages should be factual. Use a hint message for suggestions about what to do to fix the problem, especially if the suggestion might not always be applicable. It's quite common that you'll only see the actual error message in logs and displayed error messages. Having to be the owner of a relation to TRUNCATE it was an implementation detail, and one which changed back in 2008. Needing to have the replication role attribute is an implementation detail. It's not the error- the error is *permission denied to do X*. That's just plain absurd. If the user needs to explicitly configure that permission it's damned sure not just a implementation detail. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] superuser() shortcuts
* Andres Freund (and...@2ndquadrant.com) wrote: On 2014-11-26 10:18:20 -0500, Stephen Frost wrote: * Andres Freund (and...@2ndquadrant.com) wrote: I don't see how you read the contrary from the guidelines: The primary message should be short, factual, and avoid reference to implementation details such as specific function names. Short means should fit on one line under normal conditions. Use a detail message if needed to keep the primary message short, or if you feel a need to mention implementation details such as the particular system call that failed. Both primary and detail messages should be factual. Use a hint message for suggestions about what to do to fix the problem, especially if the suggestion might not always be applicable. It's quite common that you'll only see the actual error message in logs and displayed error messages. Having to be the owner of a relation to TRUNCATE it was an implementation detail, and one which changed back in 2008. Needing to have the replication role attribute is an implementation detail. It's not the error- the error is *permission denied to do X*. That's just plain absurd. If the user needs to explicitly configure that permission it's damned sure not just a implementation detail. The implementation detail is that it's not part of the normal GRANT/REVOKE privilege system, which is why it's useful to note it in the detail and why we don't need to add an errdetail along the lines of 'You must have SELECT rights on relation X to SELECT from it'. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3
Alex Shulgin a...@commandprompt.com writes: Tom Lane t...@sss.pgh.pa.us writes: Must've been my evil twin. Sorry, I must be under false impression that RhodiumToad is *your* nick on #postgresql at freenode. I don't recall who told me that, but I was pretty sure it's you. :-p That's Andrew Gierth, I believe. I'm not much for nicks; when I do use IRC, I'm tgl. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3
Alex == Alex Shulgin a...@commandprompt.com writes: Tom Lane t...@sss.pgh.pa.us writes: Must've been my evil twin. Alex Sorry, I must be under false impression that RhodiumToad is Alex *your* nick on #postgresql at freenode. I don't recall who Alex told me that, but I was pretty sure it's you. :-p ... what People do occasionally make jokes on IRC about me being Tom's clone; I know they mean it in a positive way but I still find it *extremely* annoying, so I do try and discourage it. (If they're making those same jokes elsewhere, I haven't been aware of it, but please consider this a polite public request to stop.) My first name is easily visible in the irc gecos field: *** RhodiumToad is ~andrew@[my hostname] (Andrew) and there is also the IRC users list on the wiki: http://wiki.postgresql.org/wiki/IRC2RWNames -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: plpgsql - Assert statement
2014-11-26 13:31 GMT+01:00 Marko Tiikkaja ma...@joh.to: On 11/26/14 8:55 AM, Pavel Stehule wrote: * should be assertions globally enabled/disabled? - I have no personal preference in this question. I think so. The way I would use this function is to put expensive checks into strategic locations which would only run when developing locally (and additionally perhaps in one of the test environments.) And in that case I'd like to globally disable them for the live environment. ok * can be ASSERT exception handled ? - I prefer this be unhandled exception - like query_canceled because It should not be masked by plpgsql EXCEPTION WHEN OTHERS ... I don't care much either way, as long as we get good information about what went wrong. A stack trace and hopefully something like print_strict_params for parameters to the expr. There is more ways, I can live with both Pavel .marko
Re: [HACKERS] Proposal : REINDEX SCHEMA
On Wed, Nov 26, 2014 at 3:48 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Wed, Nov 19, 2014 at 1:37 AM, Simon Riggs si...@2ndquadrant.com wrote: On 23 October 2014 00:21, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Attached patch is latest version patch I modified above. Also, I noticed I had forgotten to add the patch regarding document of reindexdb. Please don't use pg_catalog in the regression test. That way we will need to update the expected file whenever a new catalog is added, which seems pointless. Maybe create a schema with a couple of tables specifically for this, instead. These patches look fine to me. Don't see anybody objecting either. Are you looking to commit them, or shall I? IMO, SCHEMA is just but fine, that's more consistent with the existing syntax we have for database and tables. Btw, there is an error in this patch, there are no ACL checks on the schema for the user doing the REINDEX, so any user is able to perform a REINDEX on any schema. Here is an example for a given user, let's say poo': = create table foo.g (a int); ERROR: 42501: permission denied for schema foo LOCATION: aclcheck_error, aclchk.c:3371 = reindex schema foo ; NOTICE: 0: table foo.c was reindexed LOCATION: ReindexDatabaseOrSchema, indexcmds.c:1930 REINDEX A regression test to check that would be good as well. Thank you for testing this patch. It's a bug. I will fix it and add test case to regression test. Also, ISTM that it is awkward to modify the values of do_user and do_system like that in ReindexDatabase for two reasons: 1) They should be set in gram.y 2) This patch passes as a new argument of ReindexDatabase the object type, something that is overlapping with what do_user and do_system are aimed for. Why not simply defining a new object type OBJECT_SYSTEM? As this patch introduces a new object category for REINDEX, this patch seems to be a good occasion to remove the boolean dance in REINDEX at the cost of having a new object type for the concept of a system, which would be a way to define the system catalogs as a whole. +1 to define new something object type and remove do_user and do_system. But if we add OBJECT_SYSTEM to ObjectType data type, system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE. It's a bit redundant? Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject. So, I think that we need to think a bit more here. We are not far from smth that could be committed, so marking as Waiting on Author for now. Thoughts? Is the table also kind of object? Regards, --- Sawada Masahiko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Follow up to irc on CREATE INDEX vs. maintenance_work_mem on 9.3
Andrew Gierth and...@tao11.riddles.org.uk writes: Alex == Alex Shulgin a...@commandprompt.com writes: Tom Lane t...@sss.pgh.pa.us writes: Must've been my evil twin. Alex Sorry, I must be under false impression that RhodiumToad is Alex *your* nick on #postgresql at freenode. I don't recall who Alex told me that, but I was pretty sure it's you. :-p ... what People do occasionally make jokes on IRC about me being Tom's clone; I know they mean it in a positive way but I still find it *extremely* annoying, so I do try and discourage it. (If they're making those same jokes elsewhere, I haven't been aware of it, but please consider this a polite public request to stop.) My first name is easily visible in the irc gecos field: *** RhodiumToad is ~andrew@[my hostname] (Andrew) and there is also the IRC users list on the wiki: http://wiki.postgresql.org/wiki/IRC2RWNames Andrew, Tom, Sorry for the confusion. And, Andrew, thanks again for the help! :-) -- Alex -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #12070: hstore extension: hstore_to_json_loose produces invalid JSON
bo...@edookit.com writes:
The hstore_to_json_loose(hstore) produces an invalid JSON in the following
case:
SELECT hstore_to_json_loose(hstore(ARRAY ['name'], ARRAY ['1.'] :: TEXT
[]))
Output: {name: 1.}
The actual output is indeed incorrect as JSON does not permit `1.` - it must
be a string.
Yeah. The problem seems to be the ad-hoc (I'm being polite) code in
hstore_to_json_loose to decide whether a string should be treated as a
number. It does much more work than it needs to, and fails to have any
tight connection to the JSON syntax rules for numbers.
Offhand, it seems like the nicest fix would be if the core json code
exposed a function that would say whether a string matches the JSON
number syntax. Does that functionality already exist someplace,
or is it too deeply buried in the JSON parser guts?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Compiling C++ extensions on MSVC using scripts in src/tools
On 11/25/2014 11:46 PM, Michael Paquier wrote: Hi all, In the stuff I work on in a daily basis there are a couple of extensions written in C++, compiling them with MSVC on Windows using slightly-different scripts available in src/tools after copying them directly in contrib/. However, the build scripts available in src/tools/msvc are not able to detect files suffixed as cpp or similar. Attached is a two-line patch that enables their detection. I believe this would be useful for packagers on Windows. + unless ($fileNameWithPath =~ /^(.*)\\([^\\]+)\.[r]?[cyl](pp)?$/); This doesn't seem to me to be terribly well expressed (I know it's not your fault, quite possibly it's mine.) Perhaps we should replace [r]?[cyl](pp)? with (c|cpp|y|l|rc) which I think is what's intended, and seems much less obscure to me. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Compiling C++ extensions on MSVC using scripts in src/tools
Andrew Dunstan and...@dunslane.net writes: This doesn't seem to me to be terribly well expressed (I know it's not your fault, quite possibly it's mine.) Perhaps we should replace [r]?[cyl](pp)? with (c|cpp|y|l|rc) +1 ... the original coding is illegible already, not to mention wrong since it will match stuff it shouldn't. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
On Wed, Nov 26, 2014 at 10:17 AM, Palle Girgensohn gir...@pingpong.net wrote: 26 nov 2014 kl. 14:06 skrev Palle Girgensohn gir...@pingpong.net: Well, this discussion actually pushes the priority quite a bit for me -- someone else actually beeing interested about the patch... I thought it was just me... :)= By pushes the priority, I mean it gets more prioritized, in case that was unclear. :) This topic reminds me of a thread from a couple months ago: http://www.postgresql.org/message-id/f8268db6-b50f-429f-8289-da8ffa5f2...@tripadvisor.com It sounds like adding ICU support to core may also allow for adding collation versioning to indexes. Geoff -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #12070: hstore extension: hstore_to_json_loose produces invalid JSON
On 11/26/2014 11:19 AM, Tom Lane wrote:
bo...@edookit.com writes:
The hstore_to_json_loose(hstore) produces an invalid JSON in the following
case:
SELECT hstore_to_json_loose(hstore(ARRAY ['name'], ARRAY ['1.'] :: TEXT
[]))
Output: {name: 1.}
The actual output is indeed incorrect as JSON does not permit `1.` - it must
be a string.
Yeah. The problem seems to be the ad-hoc (I'm being polite) code in
hstore_to_json_loose to decide whether a string should be treated as a
number. It does much more work than it needs to, and fails to have any
tight connection to the JSON syntax rules for numbers.
Offhand, it seems like the nicest fix would be if the core json code
exposed a function that would say whether a string matches the JSON
number syntax. Does that functionality already exist someplace,
or is it too deeply buried in the JSON parser guts?
regards, tom lane
In json.c we now check numbers like this:
JsonLexContext dummy_lex;
boolnumeric_error;
...
dummy_lex.input = *outputstr == '-' ? outputstr + 1 : outputstr;
dummy_lex.input_length = strlen(dummy_lex.input);
json_lex_number(dummy_lex, dummy_lex.input, numeric_error);
numeric_error is true IFF outputstr is a legal json number.
Exposing a function to do this should be trivial.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #12070: hstore extension: hstore_to_json_loose produces invalid JSON
On 11/26/2014 11:48 AM, Andrew Dunstan wrote: In json.c we now check numbers like this: JsonLexContext dummy_lex; boolnumeric_error; ... dummy_lex.input = *outputstr == '-' ? outputstr + 1 : outputstr; dummy_lex.input_length = strlen(dummy_lex.input); json_lex_number(dummy_lex, dummy_lex.input, numeric_error); numeric_error is true IFF outputstr is a legal json number. er is NOT a legal json number cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
On Wed, Nov 26, 2014 at 6:21 AM, Greg Stark st...@mit.edu wrote: There were a number of problems with using ICU including the large dependency and the limitations of the iterator model but the main issue was that it's fundamentally a choice between being consistent with every other application on your system and being consistent with other Postgres databases running on other OSes. Most people run multiple applications on one OS, not many databases on many OSes on their own with no other applications. If Postgres used ICU then its output would be inconsistent with things like sort or ls or your application programming language's comparison operators. Unless your application programming language is written in Java, as many are. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
On Wed, Nov 26, 2014 at 2:05 AM, Dave Page dp...@postgresql.org wrote: You may want to bear in mind that postgres.app is on the main PG downloads page on the website. If you're patching Postgres to add a feature like this, it would become a fork and would have to be moved out of the PostgreSQL Core Distribution section of the download area as we only include pure distributions there. Doesn't the existing FreeBSD link go to the ports collection? And doesn't the PostgreSQL package automatically use this very ICU patch? It seems like the FreeBSD people were working around their poor OS locale support here. While I think we should officially adopt ICU, it seems a little unfair to call what they've done a fork. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test programs in contrib
Robert Haas wrote: On Wed, Nov 26, 2014 at 9:27 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Because many of them had either test in their names or some other now-useless particle, I renamed them: worker_spi - bgworker test_decoding - logical_decoding dummy_seclabel - seclabel test_shm_mq - shm_mq test_parser - tsparser I like the move. I dislike the renaming. Do you dislike the new names, or the fact that they are changing at all? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] add ssl_protocols configuration option
Alex Shulgin a...@commandprompt.com writes: I can do that too, just need a hint where to look at in libpq/psql to add the option. The place to *enforce* the option is src/interfaces/libpq/fe-secure.c (look for SSLv23_method() and SSL_CTX_set_options()). I haven't looked into how to set it. Yes, I've figured it out. Guess we'd better share the ssl_protocol value parser code between libpq and the backend. Any precedent? OK, looks like I've come up with something workable: I've added sslprotocol connection string keyword similar to pre-existing sslcompression, etc. Please see attached v2 of the original patch. I'm having doubts about the name of openssl.h header though, libpq-openssl.h? -- Alex From 7cd60e962ce5e7fc10dc52ed9c92b0b2b5c4c7f1 Mon Sep 17 00:00:00 2001 From: Alex Shulgin a...@commandprompt.com Date: Wed, 19 Nov 2014 19:49:29 +0300 Subject: [PATCH] DRAFT: ssl_protocols GUC --- doc/src/sgml/config.sgml | 29 ++ doc/src/sgml/libpq.sgml | 25 ++ src/backend/libpq/Makefile| 2 +- src/backend/libpq/be-secure-openssl.c | 29 -- src/backend/libpq/be-secure.c | 3 +- src/backend/libpq/openssl.c | 124 ++ src/backend/utils/misc/guc.c | 15 src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/libpq/libpq.h | 1 + src/include/libpq/openssl.h | 50 +++ src/interfaces/libpq/Makefile | 8 +- src/interfaces/libpq/fe-connect.c | 4 + src/interfaces/libpq/fe-secure-openssl.c | 18 +++- src/interfaces/libpq/libpq-int.h | 1 + 14 files changed, 300 insertions(+), 10 deletions(-) create mode 100644 src/backend/libpq/openssl.c create mode 100644 src/include/libpq/openssl.h diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml new file mode 100644 index ab8c263..8b701df *** a/doc/src/sgml/config.sgml --- b/doc/src/sgml/config.sgml *** include_dir 'conf.d' *** 1027,1032 --- 1027,1061 /listitem /varlistentry + varlistentry id=guc-ssl-protocols xreflabel=ssl_protocols + termvarnamessl_protocols/varname (typestring/type) + indexterm +primaryvarnamessl_protocols/ configuration parameter/primary + /indexterm + /term + listitem +para + Specifies a colon-separated list of acronymSSL/ protocols that are + allowed to be used on secure connections. Each entry in the list can + be prefixed by a literal+/ (add to the current list) + or literal-/ (remove from the current list). If no prefix is used, + the list is replaced with the specified protocol. +/para +para + The full list of supported protocols can be found in the + the applicationopenssl/ manual page. In addition to the names of + individual protocols, the following keywords can be + used: literalALL/ (all protocols supported by the underlying + crypto library), literalSSL/ (all supported versions + of acronymSSL/) and literalTLS/ (all supported versions + of acronymTLS/). +/para +para + The default is literalALL:-SSL/literal. +/para + /listitem + /varlistentry + varlistentry id=guc-ssl-ciphers xreflabel=ssl_ciphers termvarnamessl_ciphers/varname (typestring/type) indexterm diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml new file mode 100644 index e23e91d..9ea71a4 *** a/doc/src/sgml/libpq.sgml --- b/doc/src/sgml/libpq.sgml *** postgresql://%2Fvar%2Flib%2Fpostgresql/d *** 1230,1235 --- 1230,1250 /listitem /varlistentry + varlistentry id=libpq-connect-sslprotocols xreflabel=sslprotocols + termliteralsslprotocols/literal/term + listitem +para + Specifies a colon-separated list of acronymSSL/ protocols that are + allowed to be used on secure connections. + See xref linkend=guc-ssl-protocols server configuration parameter + for format. +/para +para + Defaults to literalALL:-SSL/. +/para + /listitem + /varlistentry + varlistentry id=libpq-connect-sslcompression xreflabel=sslcompression termliteralsslcompression/literal/term listitem *** myEventProc(PGEventId evtId, void *evtIn *** 6711,6716 --- 6726,6741 /para /listitem + listitem + para + indexterm +primaryenvarPGSSLPROTOCOLS/envar/primary + /indexterm + envarPGSSLPROTOCOLS/envar behaves the same as the xref + linkend=libpq-connect-sslprotocols connection parameter. + /para + /listitem + listitem para indexterm diff --git a/src/backend/libpq/Makefile
Re: [HACKERS] proposal: plpgsql - Assert statement
Hi
2014-11-26 16:46 GMT+01:00 Pavel Stehule pavel.steh...@gmail.com:
2014-11-26 13:31 GMT+01:00 Marko Tiikkaja ma...@joh.to:
On 11/26/14 8:55 AM, Pavel Stehule wrote:
* should be assertions globally enabled/disabled? - I have no personal
preference in this question.
I think so. The way I would use this function is to put expensive checks
into strategic locations which would only run when developing locally (and
additionally perhaps in one of the test environments.) And in that case
I'd like to globally disable them for the live environment.
ok
* can be ASSERT exception handled ? - I prefer this be unhandled
exception
- like query_canceled because It should not be masked by plpgsql
EXCEPTION
WHEN OTHERS ...
I don't care much either way, as long as we get good information about
what went wrong. A stack trace and hopefully something like
print_strict_params for parameters to the expr.
There is more ways, I can live with both
here is proof concept
what do you think about it?
Regards
Pavel
Pavel
.marko
commit 8c24bdca4f8cc7a0b83e5b36aaba66ce13e4d933
Author: Pavel Stehule pavel.steh...@gooddata.com
Date: Wed Nov 26 19:36:58 2014 +0100
initial
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
index 62ba092..366fcdd 100644
--- a/src/backend/utils/errcodes.txt
+++ b/src/backend/utils/errcodes.txt
@@ -454,6 +454,7 @@ PEERRCODE_PLPGSQL_ERROR plp
P0001EERRCODE_RAISE_EXCEPTIONraise_exception
P0002EERRCODE_NO_DATA_FOUND no_data_found
P0003EERRCODE_TOO_MANY_ROWS too_many_rows
+P0004EERRCODE_ASSERT_EXCEPTION assert_exception
Section: Class XX - Internal Error
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 11cb47b..3f50f54 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -136,6 +136,8 @@ static int exec_stmt_return_query(PLpgSQL_execstate *estate,
PLpgSQL_stmt_return_query *stmt);
static int exec_stmt_raise(PLpgSQL_execstate *estate,
PLpgSQL_stmt_raise *stmt);
+static int exec_stmt_assert(PLpgSQL_execstate *estate,
+PLpgSQL_stmt_assert *stmt);
static int exec_stmt_execsql(PLpgSQL_execstate *estate,
PLpgSQL_stmt_execsql *stmt);
static int exec_stmt_dynexecute(PLpgSQL_execstate *estate,
@@ -1465,6 +1467,10 @@ exec_stmt(PLpgSQL_execstate *estate, PLpgSQL_stmt *stmt)
rc = exec_stmt_raise(estate, (PLpgSQL_stmt_raise *) stmt);
break;
+ case PLPGSQL_STMT_ASSERT:
+ rc = exec_stmt_assert(estate, (PLpgSQL_stmt_assert *) stmt);
+ break;
+
case PLPGSQL_STMT_EXECSQL:
rc = exec_stmt_execsql(estate, (PLpgSQL_stmt_execsql *) stmt);
break;
@@ -3091,6 +3097,57 @@ exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
return PLPGSQL_RC_OK;
}
+/* --
+ * exec_stmt_assert Assert statement
+ * --
+ */
+static int
+exec_stmt_assert(PLpgSQL_execstate *estate, PLpgSQL_stmt_assert *stmt)
+{
+ bool value;
+ bool isnull;
+
+ value = exec_eval_boolean(estate, stmt-cond, isnull);
+ exec_eval_cleanup(estate);
+
+ if (isnull || !value)
+ {
+ StringInfoData ds;
+ char *err_hint = NULL;
+
+ initStringInfo(ds);
+
+ if (isnull)
+ appendStringInfo(ds, \%s\ is null, stmt-cond-query + 7);
+ else
+ appendStringInfo(ds, \%s\ is false, stmt-cond-query + 7);
+
+ if (stmt-hint != NULL)
+ {
+ Oid expr_typeid;
+ bool expr_isnull;
+ Datum expr_val;
+
+ expr_val = exec_eval_expr(estate, stmt-hint,
+ expr_isnull,
+ expr_typeid);
+ if (expr_isnull)
+err_hint = pstrdup(Hint is null.);
+ else
+err_hint = pstrdup(convert_value_to_string(estate, expr_val, expr_typeid));
+
+ exec_eval_cleanup(estate);
+ }
+
+ ereport(ERROR,
+(errcode(ERRCODE_ASSERT_EXCEPTION),
+ errmsg(assert_exception),
+ errdetail(ds.data),
+ (err_hint != NULL) ? errhint(%s, err_hint) : 0));
+ }
+
+ return PLPGSQL_RC_OK;
+}
/* --
* Initialize a mostly empty execution state
diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index d6825e4..1a8d00d 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -244,6 +244,8 @@ plpgsql_stmt_typename(PLpgSQL_stmt *stmt)
return RETURN QUERY;
case PLPGSQL_STMT_RAISE:
return RAISE;
+ case PLPGSQL_STMT_ASSERT:
+ return ASSERT;
case PLPGSQL_STMT_EXECSQL:
return _(SQL statement);
case PLPGSQL_STMT_DYNEXECUTE:
@@ -330,6 +332,7 @@ static void free_return(PLpgSQL_stmt_return *stmt);
static void free_return_next(PLpgSQL_stmt_return_next *stmt);
static void free_return_query(PLpgSQL_stmt_return_query *stmt);
static void free_raise(PLpgSQL_stmt_raise *stmt);
+static void free_assert(PLpgSQL_stmt_assert *stmt);
static void
[HACKERS] GSSAPI, SSPI - include_realm default
Greetings, The include_realm default for GSSAPI and SSPI is currently 'include_realm=0', meaning that the realm is stripped off of the Kerberos principal (aka the 'system' username) prior to looking up the user in pg_authid. This is fine in a single-realm environment but extremely dangerous in a multi-realm environment, as user@REALMA is rarely the same as user@REALMB. Worse, a given environment can go from single-realm to multi-realm with relative ease and most administrators aren't going to expect applications to have a problem with that change. Every other Kerberos-enabled application which I'm aware of requires either the full principal (including realm) be considered, or that the realm of the principal matches the realm of the system (which is what OpenSSH requires, as an example). As such, I'd like to propose changing the default to be 'include_realm=1'. Back when Kerberos support was originally added, we didn't have the pg_ident regex-based mapping capability. Today, users who wish to strip the realm off would be best served by configuring a mapping in pg_ident.conf which strips off exactly the realm name (or names, if they are multi-realm where the users actually are the same individuals in multiple realms) instead of using 'include_realm=0'. Users who really wish to strip off the realm for their environment would still be able to add 'include_realm=0' to their pg_hba.conf. We would recommend against that in the documentation, however, and explain how it's unsafe. I would recommend that this be coached as transistional support for users who wish to upgrade but don't want to (further) change their configuration immediately, with the implication that we might remove it some day. This would be done for 9.5 and we would need to note it in the release notes, of course. Shipping an insecure pg_hba.conf as the default (with 'trust') works because the distributions change it to a more secure setting anyway. There's no similar option to change the default for include_realm short of hacking the source code and documentation, which would be much more invasive and likely invite complaints from users when their configuration doesn't work the way the postgresql.org docs claim it should. Thoughts? Thanks! Stephen signature.asc Description: Digital signature
[HACKERS] Caching negative lookup results in typcache.c
In the pgsql-performance thread at http://www.postgresql.org/message-id/flat/CAOR=d=3j1U_q-zf8+jUx1hkx8ps+N8pm=EUTqyFdJ5ov=+f...@mail.gmail.com it emerged that a substantial part of the slowdown Scott saw from 8.4 to 9.2 was an unexpected consequence of the fact that the planner now considers hashing methods for implementing UNION. That leads to requests to typcache.c to look up the hash opclass for a UNION'd datatype. Which is fine, and fast, as long as there is one ... but if there is not, each request causes a new physical probe into pg_opclass, because typcache.c is not designed to cache negative results. Fixing that results in about a 16% reduction in total query time on Scott's example (for me, 432 ms to 360 ms, in a cassert-off build). We learned years ago that it was critical for the catalog caches to remember negative lookup results, so I'm a bit surprised that it's taken us this long to realize that typcache.c needs to do it too. If memory serves, the reason for not caching negative results was the thought that opclasses might get added to a type over the course of a session. However, that's pretty lame logic, because they could also get dropped over the course of a session, and the existing typcache.c code utterly fails to cope with that scenario; it could easily result in attempts to call no-longer-existing operators/functions. The attached proposed patch adjusts the typcache.c code so that negative results are cacheable, and adds a syscache invalidation callback so that all such results are forgotten whenever an update occurs in pg_opclass. That's sufficient to cover the cases of CREATE/DROP OPERATOR CLASS, which are the only supported scenarios for altering operator classes. I think it is not necessary to watch for pg_amop or pg_amproc updates, because the supported uses of ALTER OPERATOR FAMILY ADD/DROP OPERATOR/FUNCTION are for cross-type family members, none of which are cached in typcache. Comments? Is it reasonable to think of back-patching this? I'm inclined to think that performance is not a sufficient argument for doing that, because the lossage occurs only for types that have a default btree opclass but no default hash opclass or vice versa; and that is a fairly short list, with no heavily-used types involved. However, failure to honor DROP OPERATOR CLASS is clearly a bug, and that might be a good argument for back-patching (though I've not heard any field complaints about it). regards, tom lane diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 8c6c7fc..9aa9182 100644 *** a/src/backend/utils/cache/typcache.c --- b/src/backend/utils/cache/typcache.c *** *** 17,36 * is kept in the type cache. * * Once created, a type cache entry lives as long as the backend does, so ! * there is no need for a call to release a cache entry. (For present uses, * it would be okay to flush type cache entries at the ends of transactions, * if we needed to reclaim space.) * ! * There is presently no provision for clearing out a cache entry if the ! * stored data becomes obsolete. (The code will work if a type acquires ! * opclasses it didn't have before while a backend runs --- but not if the ! * definition of an existing opclass is altered.) However, the relcache ! * doesn't cope with opclasses changing under it, either, so this seems ! * a low-priority problem. ! * ! * We do support clearing the tuple descriptor and operator/function parts ! * of a rowtype's cache entry, since those may need to change as a consequence ! * of ALTER TABLE. * * * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group --- 17,32 * is kept in the type cache. * * Once created, a type cache entry lives as long as the backend does, so ! * there is no need for a call to release a cache entry. If the type is ! * dropped, the cache entry simply becomes wasted storage. (For present uses, * it would be okay to flush type cache entries at the ends of transactions, * if we needed to reclaim space.) * ! * We have some provisions for updating cache entries if the stored data ! * becomes obsolete. Information dependent on opclasses is cleared if we ! * detect updates to pg_opclass. We also support clearing the tuple ! * descriptor and operator/function parts of a rowtype's cache entry, ! * since those may need to change as a consequence of ALTER TABLE. * * * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group *** static HTAB *TypeCacheHash = NULL; *** 77,82 --- 73,85 #define TCFLAGS_CHECKED_FIELD_PROPERTIES 0x0010 #define TCFLAGS_HAVE_FIELD_EQUALITY 0x0020 #define TCFLAGS_HAVE_FIELD_COMPARE 0x0040 + #define TCFLAGS_CHECKED_BTREE_OPCLASS 0x0080 + #define TCFLAGS_CHECKED_HASH_OPCLASS 0x0100 + #define TCFLAGS_CHECKED_EQ_OPR0x0200 + #define TCFLAGS_CHECKED_LT_OPR0x0400 + #define
Re: [HACKERS] GSSAPI, SSPI - include_realm default
On 11/26/14 2:01 PM, Stephen Frost wrote: As such, I'd like to propose changing the default to be 'include_realm=1'. Sounds reasonable to me. include_realm is supported back to 8.4, so affected users can set include_realm=0 in their existing installations. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSSAPI, SSPI - include_realm default
* Peter Eisentraut (pete...@gmx.net) wrote: On 11/26/14 2:01 PM, Stephen Frost wrote: As such, I'd like to propose changing the default to be 'include_realm=1'. Sounds reasonable to me. include_realm is supported back to 8.4, so affected users can set include_realm=0 in their existing installations. Ah, yes, good point. Will include that suggestion also. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] 9.2 recovery/startup problems
On Wed, Nov 26, 2014 at 5:06 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Jeff Janes wrote: This is what I get in the log from the attempted restart: PST LOG: database system was interrupted; last known up at 2014-11-25 15:40:33 PST PST LOG: database system was not properly shut down; automatic recovery in progress PST LOG: redo starts at 84/EF80 PST LOG: record with zero length at 84/EF09AE18 PST LOG: redo done at 84/EF09AD28 PST LOG: last completed transaction was at log time 2014-11-25 15:42:09.173599-08 PST LOG: checkpoint starting: end-of-recovery immediate PST LOG: checkpoint complete: wrote 103 buffers (0.2%); 0 transaction log file(s) added, 246 removed, 7 recycled; write=0.002 s, sync=0.020 s, total=0.526 s; sync files=51, longest=0.003 s, average=0.000 s PST FATAL: could not create file base/16416/59288: File exists Maybe fire up pg_xlogdump to see what xlog record references that relfilenode. pg_xlogdump doesn't exist yet in 9.2 (or can I use a newer one against the older files?). Immediately after the reboot, base/16416/59288 exists but is empty. After the failed start up attempt, base/16416/59288 and base/16416/59288_init both exist and are empty, with base/16416/59288 having its pre-startup-attempt timestamp. So it seems like the code that copies the init fork over the main fork at the end of crash recovery is doing something wrong in this case. But I don't understand why it then succeeds at starting up the next time I try it. If I use Snaga's xlogdump (which I'm not sure is entirely correct under 9.2), the end of the stream looks like this: [cur:85/1008BD88, xid:4547590, rmid:10(Heap), len/tot_len:174/206, info:64, prev:85/1008BD58] hot_update: s/d/r:1663/16416/12636 block 16 off 41 to block 16 off 43 [cur:85/1008BE58, xid:4547590, rmid:2(Storage), len/tot_len:16/48, info:16, prev:85/1008BD88] create rel: s/d/r:1663/16416/59288 [cur:85/1008BE88, xid:4547590, rmid:8(Standby), len/tot_len:16/48, info:0, prev:85/1008BE58] standby [page:70, xlp_info:5, xlp_tli:2, xlp_pageaddr:85/1008C000] XLP_FIRST_IS_CONTRECORD XLP_BKP_REMOVABLE [cur:85/1008BEB8, xid:4547590, rmid:10(Heap), len/tot_len:28/7160, info:72, prev:85/1008BE88] hot_update: s/d/r:1663/16416/12636 block 30 off 32 to block 30 off 33 [cur:85/1008BEB8, xid:4547590, rmid:10(Heap), len/tot_len:28/7160, info:72, prev:85/1008BE88] bkpblock[1]: s/d/r:1663/16416/12636 blk:30 hole_off/len:156/1116 Bogus page magic number at offset 8E000 Thanks, Jeff
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
On 11/26/14 12:46 PM, Peter Geoghegan wrote: On Wed, Nov 26, 2014 at 2:05 AM, Dave Page dp...@postgresql.org wrote: You may want to bear in mind that postgres.app is on the main PG downloads page on the website. If you're patching Postgres to add a feature like this, it would become a fork and would have to be moved out of the PostgreSQL Core Distribution section of the download area as we only include pure distributions there. Doesn't the existing FreeBSD link go to the ports collection? And doesn't the PostgreSQL package automatically use this very ICU patch? It seems like the FreeBSD people were working around their poor OS locale support here. While I think we should officially adopt ICU, it seems a little unfair to call what they've done a fork. I would welcome the addition of support for ICU and possibly other locale libraries. The features were designed with that in mind. But I think what is being proposed here needs to be reigned in from time to time. Search the archives at various times for debian, gentoo, or even mandrake for examples of what can happen when this goes too far. It's a sliding scale. FreeBSD ports are notionally a build-from-source system targeted as experts. Someone who installs a port has a chance to look at the port definition and learn what will be installed. (A build option and a more explicit warning might be nice.) Postgres.app is a binary distribution apparently targeted at inexperienced or casual users at a much bigger scale. Users won't have an option to learn about this unofficial feature or a chance to disable it. Also, Postgres.app is not the only distribution for this platform, so this could create a lot of confusion. It's open source, and I don't want to discourage people from experimenting and sharing. But I'm with Dave: listing a distribution among the primary download options should imply that the software is as pristine as possible. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] no test programs in contrib
On 11/26/14 9:27 AM, Alvaro Herrera wrote: I haven't done anything about documentation. I thought a new chapter after Additional Supplied Modules, perhaps entitled Additional Sample Modules would be appropriate. I would remove the SGML files and put simple README files into each directory. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] bug in json_to_record with arrays
Tested on 9.4b3, 9.4rc1, 9.5devel
select * from json_to_record('
{id:1,val:josh,valry:[potter,chef,programmer]}') as r(id
int, val text, valry text[]);
ERROR: missing dimension value
With some experimentation, I can't find any way to convert a JSON array
to an array field using json_to_record or json_to_recordset. I know
this worked back in January, though.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
On 11/26/2014 11:54 AM, Josh Berkus wrote:
Tested on 9.4b3, 9.4rc1, 9.5devel
select * from json_to_record('
{id:1,val:josh,valry:[potter,chef,programmer]}') as r(id
int, val text, valry text[]);
ERROR: missing dimension value
With some experimentation, I can't find any way to convert a JSON array
to an array field using json_to_record or json_to_recordset. I know
this worked back in January, though.
Lemme take that back, it didn't work. Just checked an old devel snapshot.
Looks like this is not intended to work, so the only bug is that we need
a less confusing error message.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSSAPI, SSPI - include_realm default
On Wed, Nov 26, 2014 at 8:01 PM, Stephen Frost sfr...@snowman.net wrote: Greetings, The include_realm default for GSSAPI and SSPI is currently 'include_realm=0', meaning that the realm is stripped off of the Kerberos principal (aka the 'system' username) prior to looking up the user in pg_authid. This is fine in a single-realm environment but extremely dangerous in a multi-realm environment, as user@REALMA is rarely the same as user@REALMB. Worse, a given environment can go from single-realm to multi-realm with relative ease and most administrators aren't going to expect applications to have a problem with that change. Every other Kerberos-enabled application which I'm aware of requires either the full principal (including realm) be considered, or that the realm of the principal matches the realm of the system (which is what OpenSSH requires, as an example). As such, I'd like to propose changing the default to be 'include_realm=1'. Per our previous discussions, but to make sure it's also on record for others, +1 for this suggestion. Back when Kerberos support was originally added, we didn't have the pg_ident regex-based mapping capability. Today, users who wish to strip the realm off would be best served by configuring a mapping in pg_ident.conf which strips off exactly the realm name (or names, if they are multi-realm where the users actually are the same individuals in multiple realms) instead of using 'include_realm=0'. Users who really wish to strip off the realm for their environment would still be able to add 'include_realm=0' to their pg_hba.conf. We would recommend against that in the documentation, however, and explain how it's unsafe. I would recommend that this be coached as transistional support for users who wish to upgrade but don't want to (further) change their configuration immediately, with the implication that we might remove it some day. This would be done for 9.5 and we would need to note it in the release notes, of course. I suggest we also backpatch some documentation suggesting that people manually change the include_realm parameter (perhaps also with a note saying that the default will change in 9.5). -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSSAPI, SSPI - include_realm default
* Magnus Hagander (mag...@hagander.net) wrote: On Wed, Nov 26, 2014 at 8:01 PM, Stephen Frost sfr...@snowman.net wrote: This would be done for 9.5 and we would need to note it in the release notes, of course. I suggest we also backpatch some documentation suggesting that people manually change the include_realm parameter (perhaps also with a note saying that the default will change in 9.5). Agreed and will do (this was also suggested by Peter). Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
26 nov 2014 kl. 20:42 skrev Peter Eisentraut pete...@gmx.net: (A build option and a more explicit warning might be nice.) In the freebsd ports, it is an option, default is off. :-) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [pgsql-packagers] Palle Girgensohn's ICU patch
On 11/26/14 3:42 PM, Palle Girgensohn wrote: 26 nov 2014 kl. 20:42 skrev Peter Eisentraut pete...@gmx.net: (A build option and a more explicit warning might be nice.) In the freebsd ports, it is an option, default is off. :-) That's even better. Sorry, I looked at the port sources and couldn't identify that it was an option. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
Josh Berkus j...@agliodbs.com writes:
On 11/26/2014 11:54 AM, Josh Berkus wrote:
Tested on 9.4b3, 9.4rc1, 9.5devel
select * from json_to_record('
{id:1,val:josh,valry:[potter,chef,programmer]}') as r(id
int, val text, valry text[]);
ERROR: missing dimension value
With some experimentation, I can't find any way to convert a JSON array
to an array field using json_to_record or json_to_recordset. I know
this worked back in January, though.
Lemme take that back, it didn't work. Just checked an old devel snapshot.
Looks like this is not intended to work, so the only bug is that we need
a less confusing error message.
What's happening is that this string:
[potter,chef,programmer]
is getting passed to array_in, which of course does not like it because
that's not the I/O syntax for Postgres arrays. Arguably,
populate_record_worker should be smart enough to convert somehow, but
it isn't today. Looks to me like it wouldn't succeed for the comparable
case of converting a sub-object to a Postgres composite type, either.
I'm satisfied with regarding those cases as missing features to be
added later.
As far as your request for a better error message is concerned, I'm a
bit inclined to lay the blame on array_in rather than the JSON code.
Wouldn't it be better if it said
ERROR: invalid input syntax for array: [potter,chef,programmer]
DETAIL: Dimension value is missing.
which is comparable to what you'd get out of most other input functions
that were feeling indigestion?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.2 recovery/startup problems
Hi, On 2014-11-26 11:29:09 -0800, Jeff Janes wrote: On Wed, Nov 26, 2014 at 5:06 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Jeff Janes wrote: This is what I get in the log from the attempted restart: PST LOG: database system was interrupted; last known up at 2014-11-25 15:40:33 PST PST LOG: database system was not properly shut down; automatic recovery in progress PST LOG: redo starts at 84/EF80 PST LOG: record with zero length at 84/EF09AE18 PST LOG: redo done at 84/EF09AD28 PST LOG: last completed transaction was at log time 2014-11-25 15:42:09.173599-08 PST LOG: checkpoint starting: end-of-recovery immediate PST LOG: checkpoint complete: wrote 103 buffers (0.2%); 0 transaction log file(s) added, 246 removed, 7 recycled; write=0.002 s, sync=0.020 s, total=0.526 s; sync files=51, longest=0.003 s, average=0.000 s PST FATAL: could not create file base/16416/59288: File exists Any chance you can reproduce this in a reproducible fashion? Maybe fire up pg_xlogdump to see what xlog record references that relfilenode. pg_xlogdump doesn't exist yet in 9.2 (or can I use a newer one against the older files?). Nope, you can't :( Immediately after the reboot, base/16416/59288 exists but is empty. After the failed start up attempt, base/16416/59288 and base/16416/59288_init both exist and are empty, with base/16416/59288 having its pre-startup-attempt timestamp. So it seems like the code that copies the init fork over the main fork at the end of crash recovery is doing something wrong in this case. But I don't understand why it then succeeds at starting up the next time I try it. If I use Snaga's xlogdump (which I'm not sure is entirely correct under 9.2), the end of the stream looks like this: [cur:85/1008BD88, xid:4547590, rmid:10(Heap), len/tot_len:174/206, info:64, prev:85/1008BD58] hot_update: s/d/r:1663/16416/12636 block 16 off 41 to block 16 off 43 [cur:85/1008BE58, xid:4547590, rmid:2(Storage), len/tot_len:16/48, info:16, prev:85/1008BD88] create rel: s/d/r:1663/16416/59288 [cur:85/1008BE88, xid:4547590, rmid:8(Standby), len/tot_len:16/48, info:0, prev:85/1008BE58] standby [page:70, xlp_info:5, xlp_tli:2, xlp_pageaddr:85/1008C000] XLP_FIRST_IS_CONTRECORD XLP_BKP_REMOVABLE [cur:85/1008BEB8, xid:4547590, rmid:10(Heap), len/tot_len:28/7160, info:72, prev:85/1008BE88] hot_update: s/d/r:1663/16416/12636 block 30 off 32 to block 30 off 33 [cur:85/1008BEB8, xid:4547590, rmid:10(Heap), len/tot_len:28/7160, info:72, prev:85/1008BE88] bkpblock[1]: s/d/r:1663/16416/12636 blk:30 hole_off/len:156/1116 Hm. It looks like the problem here might be an ordering one. Notice that there seems to be a record for the main relation, but not the init fork in the excerpt. It wonder if ExcecuteTruncate() does things in the wrong order. /* * Need the full transaction-safe pushups. * * Create a new empty storage file for the relation, and assign it * as the relfilenode value. The old storage file is scheduled for * deletion at commit. */ RelationSetNewRelfilenode(rel, RecentXmin, minmulti); if (rel-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED) heap_create_init_fork(rel); Arguably that can cause problems if the node is promoted between the RelationSetNewRelfilenode() and the heap_create_init_fork(). On the other hand, the fork should essentially be 'invisible' in that case as the transaction won't commit... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
On 11/26/2014 03:48 PM, Tom Lane wrote: Arguably, populate_record_worker should be smart enough to convert somehow, but it isn't today. Looks to me like it wouldn't succeed for the comparable case of converting a sub-object to a Postgres composite type, either. I'm satisfied with regarding those cases as missing features to be added later. Right. Before commit a749a23d7af4dba9b3468076ec561d2cbf69af09 it didn't try by default to treat the value as text, but instead errored out if the value was an array or object, with an appropriate message. Now we always try to treat it as text and pass that to the array or composite input functions, who now get the responsibility of telling us what's wrong. It might be possible to process such values recursively, but it would be far from trivial. As far as your request for a better error message is concerned, I'm a bit inclined to lay the blame on array_in rather than the JSON code. Wouldn't it be better if it said ERROR: invalid input syntax for array: [potter,chef,programmer] DETAIL: Dimension value is missing. which is comparable to what you'd get out of most other input functions that were feeling indigestion? +1 cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
* Tom Lane (t...@sss.pgh.pa.us) wrote: As far as your request for a better error message is concerned, I'm a bit inclined to lay the blame on array_in rather than the JSON code. Wouldn't it be better if it said ERROR: invalid input syntax for array: [potter,chef,programmer] DETAIL: Dimension value is missing. Sounds pretty reasonable to me, but I would just caution that we should check if that's considered 'leakproof' or not (or, if it is, if it'd ever possibly leak data it shouldn't or if it would only ever return information provided by the user). Otherwise, someone might be able to convince the planner to push it down below a security qual and expose data from rows which shouldn't be visible. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] memory explosion on planning complex query
On Wed, Nov 26, 2014 at 2:00 PM, Andrew Dunstan and...@dunslane.net wrote: The client's question is whether this is not a bug. It certainly seems like it should be possible to plan a query without chewing up this much memory, or at least to be able to limit the amount of memory that can be grabbed during planning. Going from humming along happily to OOM conditions all through running explain somequery is not very friendly. Have you tried this with a #define SHOW_MEMORY_STATS build, or otherwise rigged Postgres to call MemoryContextStats() at interesting times? -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] memory explosion on planning complex query
On 26.11.2014 23:26, Peter Geoghegan wrote: On Wed, Nov 26, 2014 at 2:00 PM, Andrew Dunstan and...@dunslane.net wrote: The client's question is whether this is not a bug. It certainly seems like it should be possible to plan a query without chewing up this much memory, or at least to be able to limit the amount of memory that can be grabbed during planning. Going from humming along happily to OOM conditions all through running explain somequery is not very friendly. Have you tried this with a #define SHOW_MEMORY_STATS build, or otherwise rigged Postgres to call MemoryContextStats() at interesting times? FWIW, this does the trick on a regular build: gdb -batch -x gdb.cmd -p $PID where gdb.cmd is a file with a single line: p MemoryContextStats(TopMemoryContext) Just execute it at the interesting moment when a lot of memory is consumed. Tomas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] memory explosion on planning complex query
On 11/26/2014 11:00 PM, Andrew Dunstan wrote: Attached is some anonymized DDL for a fairly complex schema from a PostgreSQL Experts client. Also attached is an explain query that runs against the schema. The client's problem is that in trying to run the explain, Postgres simply runs out of memory. On my untuned 9.3 test rig, (Scientific Linux 6.4 with 24Gb of RAM and 24Gb of swap) vmstat clearly shows the explain chewing up about 7Gb of memory. When it's done the free memory jumps back to where it was. On a similar case on the clients test rig we saw memory use jump lots more. The client's question is whether this is not a bug. It certainly seems like it should be possible to plan a query without chewing up this much memory, or at least to be able to limit the amount of memory that can be grabbed during planning. Going from humming along happily to OOM conditions all through running explain somequery is not very friendly. It's not trivial to track the whole hierarchy of views, but I think it can result in the FROM list or some JOIN lists being too long. How about setting from_collapse_limit / join_collapse_limit to lower-than-default value ? -- Antonin Houska Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt Web: http://www.postgresql-support.de, http://www.cybertec.at -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] memory explosion on planning complex query
On 11/26/2014 05:26 PM, Peter Geoghegan wrote: On Wed, Nov 26, 2014 at 2:00 PM, Andrew Dunstan and...@dunslane.net wrote: The client's question is whether this is not a bug. It certainly seems like it should be possible to plan a query without chewing up this much memory, or at least to be able to limit the amount of memory that can be grabbed during planning. Going from humming along happily to OOM conditions all through running explain somequery is not very friendly. Have you tried this with a #define SHOW_MEMORY_STATS build, or otherwise rigged Postgres to call MemoryContextStats() at interesting times? No, but I can. Good idea, thanks. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug in json_to_record with arrays
Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: As far as your request for a better error message is concerned, I'm a bit inclined to lay the blame on array_in rather than the JSON code. Wouldn't it be better if it said ERROR: invalid input syntax for array: [potter,chef,programmer] DETAIL: Dimension value is missing. Sounds pretty reasonable to me, but I would just caution that we should check if that's considered 'leakproof' or not (or, if it is, if it'd ever possibly leak data it shouldn't or if it would only ever return information provided by the user). array_in could only be regarded as leakproof if every element-type input function it could ever call is also leakproof. Which ain't the case, so I sure hope it's not marked that way. (Likewise record_in, range_in, etc.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Maximum number of WAL files in the pg_xlog directory
On Mon, Nov 3, 2014 at 12:39:26PM -0800, Jeff Janes wrote: It looked to me that the formula, when descending from a previously stressed state, would be: greatest(1 + checkpoint_completion_target) * checkpoint_segments, wal_keep_segments) + 1 + 2 * checkpoint_segments + 1 I don't think we can assume checkpoint_completion_target is at all reliable enough to base a maximum calculation on, assuming anything above the maximum is cause of concern and something to inform the admins about. Assuming checkpoint_completion_target is 1 for maximum purposes, how about: max(2 * checkpoint_segments, wal_keep_segments) + 2 * checkpoint_segments + 2 -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.2 recovery/startup problems
On Thu, Nov 27, 2014 at 4:29 AM, Jeff Janes jeff.ja...@gmail.com wrote: pg_xlogdump doesn't exist yet in 9.2 (or can I use a newer one against the older files?). Not sure if pg_xlogdump would work (9.5 not for sure, 9.4 should partially, 9.3 has better chances), but you could try this one as well that is compatible with 9.2: https://github.com/snaga/xlogdump -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] What exactly is our CRC algorithm?
On Fri, Nov 21, 2014 at 07:49:45AM -0600, k...@rice.edu wrote: Hi, This indicates that another part of the system is the resource limit, not the CRC calculation. My money is on the I/O system. Try it using an in memory filesystem and see if that matters. Even if it is still the same results, at least the new version of CRC is no worse than the current version. I would test it with fsync=off to remove the fsync delay. That will simulate an SSD or BBU controller. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.2 recovery/startup problems
On Tue, Nov 25, 2014 at 9:30 PM, Jeff Janes jeff.ja...@gmail.com wrote: Using both 9.2.9 and 9_2_STABLE 9b468bcec15f1ea7433d4, I get a fairly reproducible startup failure. What I was doing is restore a database from a base backup and roll it forward with a recovery.conf until it completes and starts up. Then I truncate an unlogged table and start repopulating it with a large slow 'insert into ...select from' query. While that is running, if I reboot the server, when it restarts it does not come back up initially. This is what I get in the log from the attempted restart: PST LOG: database system was interrupted; last known up at 2014-11-25 15:40:33 PST PST LOG: database system was not properly shut down; automatic recovery in progress PST LOG: redo starts at 84/EF80 PST LOG: record with zero length at 84/EF09AE18 PST LOG: redo done at 84/EF09AD28 PST LOG: last completed transaction was at log time 2014-11-25 15:42:09.173599-08 PST LOG: checkpoint starting: end-of-recovery immediate PST LOG: checkpoint complete: wrote 103 buffers (0.2%); 0 transaction log file(s) added, 246 removed, 7 recycled; write=0.002 s, sync=0.020 s, total=0.526 s; sync files=51, longest=0.003 s, average=0.000 s PST FATAL: could not create file base/16416/59288: File exists PST LOG: startup process (PID 2472) exited with exit code 1 PST LOG: aborting startup due to startup process failure oid2name doesn't show me any 59288, so I think it is the new copy of the unlogged table which is being created at the moment of the reboot. 59288 is not the new (uncommitted) unlogged table itself, but is the new (uncommitted) toast table associated with that unlogged table. immediately before the 'sudo /sbin/reboot', while the 'truncate unlogged_table; insert into unlogged_table from select...' is running, the two files below exist and have zero size. base/16416/59288_init base/16416/59288 Immediately after the system has come back up, the file base/16416/59288_init no longer exists. I can't reproduce this behavior without the reboot. It seems to depend on the sequencing and timing of the signals which the kernel delivers to the running processes during the reboot. If I do a pg_ctl stop -mf, then both files go away. If I do a pg_ctl stop -mi, then neither goes away. It is only with the /sbin/reboot that I get the fatal combination of _init being gone but the other still present. Then once the system is back and I restart postmaster, the first pass through ResetUnloggedRelationsInDbspaceDir doesn't remove base/16416/59288, because base/16416/59288_init doesn't exist to trigger the deletion. On the 2nd pass through, base/16416/59288_init exists because it was created by WAL replay, and the copy fails because it is expecting base/16416/59288 to have already been cleaned up by the first pass. Should ResetUnloggedRelationsInDbspaceDir on the second pass attempt to unlink the target immediately before the copy (line 338, in 9.2) to accommodate cases like this? Cheers, Jeff
Re: [HACKERS] memory explosion on planning complex query
On 11/26/2014 05:00 PM, Andrew Dunstan wrote: Attached is some anonymized DDL for a fairly complex schema from a PostgreSQL Experts client. Also attached is an explain query that runs against the schema. The client's problem is that in trying to run the explain, Postgres simply runs out of memory. On my untuned 9.3 test rig, (Scientific Linux 6.4 with 24Gb of RAM and 24Gb of swap) vmstat clearly shows the explain chewing up about 7Gb of memory. When it's done the free memory jumps back to where it was. On a similar case on the clients test rig we saw memory use jump lots more. The client's question is whether this is not a bug. It certainly seems like it should be possible to plan a query without chewing up this much memory, or at least to be able to limit the amount of memory that can be grabbed during planning. Going from humming along happily to OOM conditions all through running explain somequery is not very friendly. Further data point - thanks to Andrew Gierth (a.k.a. RhodiumToad) for pointing this out. The query itself grabs about 600Mb to 700Mb to run, whereas the EXPLAIN takes vastly more - on my system 10 times more. Surely that's not supposed to happen? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add CREATE support to event triggers
On Sat, Nov 8, 2014 at 05:56:00PM +0100, Andres Freund wrote: On 2014-11-08 11:52:43 -0500, Tom Lane wrote: Adding a similar level of burden to support a feature with a narrow use-case seems like a nonstarter from here. I don't understand this statement. In my experience the lack of a usable replication solution that allows temporary tables and major version differences is one of the most, if not *the* most, frequent criticisms of postgres I hear. How is this a narrow use case? How would replicating DDL handle cases where the master and slave servers have different major versions and the DDL is only supported by the Postgres version on the master server? If it would fail, does this limit the idea that logical replication allows major version-different replication? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add CREATE support to event triggers
Bruce Momjian wrote: On Sat, Nov 8, 2014 at 05:56:00PM +0100, Andres Freund wrote: On 2014-11-08 11:52:43 -0500, Tom Lane wrote: Adding a similar level of burden to support a feature with a narrow use-case seems like a nonstarter from here. I don't understand this statement. In my experience the lack of a usable replication solution that allows temporary tables and major version differences is one of the most, if not *the* most, frequent criticisms of postgres I hear. How is this a narrow use case? How would replicating DDL handle cases where the master and slave servers have different major versions and the DDL is only supported by the Postgres version on the master server? Normally you would replicate between an older master and a newer replica, so this shouldn't be an issue. I find it unlikely that we would de-support some syntax that works in an older version: it would break pg_dump, for one thing. In other words I view cross-version replication as a mechanism to upgrade, not something that you would use permanently. Once you finish upgrading, promote the newer server and ditch the old master. If it would fail, does this limit the idea that logical replication allows major version-different replication? Not in my view, at least. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add CREATE support to event triggers
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Bruce Momjian wrote: How would replicating DDL handle cases where the master and slave servers have different major versions and the DDL is only supported by the Postgres version on the master server? Normally you would replicate between an older master and a newer replica, so this shouldn't be an issue. I find it unlikely that we would de-support some syntax that works in an older version: it would break pg_dump, for one thing. While I tend to agree with you that it's not something we're likely to do, how would it break pg_dump? We have plenty of version-specific logic in pg_dump and we could certainly generate a different syntax in a newer version than we did in an older version, if the newer server was expecting something different. We've always held that you should use the version of pg_dump which match the server you are moving *to*, after all. In other words I view cross-version replication as a mechanism to upgrade, not something that you would use permanently. Once you finish upgrading, promote the newer server and ditch the old master. I agree with this also. If it would fail, does this limit the idea that logical replication allows major version-different replication? Not in my view, at least. I'm all for having logical replication be a way to do major version different replication (particularly for the purposes of upgrades), but it shouldn't mean we can never de-support a given syntax. As one example, we've discussed a few times removing certain table-level privileges on the basis that they practically mean you own the table. Perhaps that'll never actually happen, but if it does, logical replication would need to deal with it. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Add shutdown_at_recovery_target option to recovery.conf
On Wed, Nov 26, 2014 at 7:10 PM, Christoph Berg c...@df7cb.de wrote: Re: Petr Jelinek 2014-11-25 5474efea.2040...@2ndquadrant.com Patch committed. Thanks! I'm a bit late to the party, but wouldn't recovery_target_action = ... have been a better name for this? It'd be in line with the other recovery_target_* parameters, and also a bit shorter than the imho somewhat ugly action_at_recovery_target. Looks like a good idea to me. Why not as well mark pause_at_recovery_target as deprecated in the docs and remove it in, let's say, 2 releases? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dblink_get_connections() result for no connections
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/25/2014 12:49 PM, Tom Lane wrote: While fooling around with the array_agg(anyarray) patch, I noted that dblink_get_connections() returns NULL if there are no active connections. It seems like an empty array might be a saner definition --- thoughts? (Sorry for the slow response -- was traveling all day yesterday) The original patch introducing this function is seen here: http://www.postgresql.org/message-id/44c50f4a.7050...@joeconway.com and committed by me here: http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=52a3ed9fac32e16f6061cbc49046c0bd97f8f77a In particular, the original author said: 8- Added functions: [...] dblink_get_connections() List all open dblink connections by name. Returns a comma separated string of all connection names. Takes no params Example: SELECT string_to_array(dblink_get_connections(), ','); 8- He really did not specify what the return value should be if there are no connections. Somewhere between submission and commit I changed that to return an array rather than comma separated string. To be honest I have no idea why I did not make it an empty array instead of a NULL in that case, but that does make more sense to me now that you mention it. Might cause some breakage for people depending on that though... Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 Support -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQIcBAEBAgAGBQJUdphsAAoJEDfy90M199hlXuUP/i72+AKHdDS18gjHv7/axvS1 /6v0KMK9GvQBbfaHHlQ22eoCBkX6eZI3XW7FYymD4oDYBEK3rihck6sjBxFokrK7 QSYWiSX0tgeAvE99WCI3aLxyXZDTxnlaobfTwg1OMUsz2a8O23fppDrevYe+4EDC t40dpiJiG/6r7wektARSJQG0DtINaEeRwvGVBhMExvGo+76GNfotqTaRLex0Fugi 8FoE7VP61Kx9P7wMV/QOB3VK+apfPVEd4jN6lsSYp0MDKI64dk6ITfxng9KTMbUi wP+01WK7oljJeUBQRM1Co/OBXcAUTWXhr6wwwZgUqw1eeMU7rXLuDDtIz4B2NEFY tp6mFuAb15x+BmdBjJSYTz/ckG7n+NEJ4VyiK/gQWe3+WUajRqAPh38xpiWniTL4 l7cajIAWy5kO7RKXyaFxmRqXI148lUO2DosF/uL0nbOrCYjdf9VSGnDzq9bST2BE mz7i7l5Ej/Pt1OPu4oeKRyEo39GkyD/2fsstVv66403LYaEWwyyKUA2rg1W1T2aN WALXUgCmpEVstTtHJFS6cvGy4dw4QobUihJWOZEOtesF7XIxQcXLttTUrydPBfVC 6zOZIoVcKO1O2ER0nFBmSVVo+63Y4rgK8WLnJJ2C7WcFvC0Kd5BfzQ1BSMoPeCA2 2KhIlr2QbyeUeY6Ebz+o =vNw9 -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
