Re: [HACKERS] nodes/*funcs.c inconsistencies
On Sun, Aug 02, 2015 at 11:31:16PM -0400, Tom Lane wrote: Stephen Frost sfr...@snowman.net writes: Noah, A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic INT/UINT or field order corrections. The non-cosmetic changes involve CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the existing treatments (ignore custom_plans/custom_paths fields; copy/compare cmd string pointer as a scalar) were deliberate, please let me know. Thanks for the review. The change you have is correct for CreatePolicyStmt, at least. I imagine I confused it with polcmd, which is actually just a char. Barring objections, I'll change it to cmd_name after your commit, to reduce the chances of future confusion. The existing identifier seems fine, but won't I mind that change, either. Both of you please keep in mind that these cosmetic changes are initdb-forcing, at least if they affect node types that can appear in stored rules. Right; Stephen's does not force initdb, but some of what I posted does so. That being the case, it would probably be a good idea to get them done before alpha2, as there may not be a good opportunity afterwards. Freedom to bump catversion after alpha2 will be barely-distinguishable from freedom to do so now. I have planned to leave my usual comment period of a few days, though skipping that would be rather innocuous in this case. -- 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] pg_rewind failure by file deletion in source server
On Sun, Aug 2, 2015 at 4:01 AM, Heikki Linnakangas wrote: Perhaps it's best if we copy all the WAL files from source in copy-mode, but not in libpq mode. Regarding old WAL files in the target, it's probably best to always leave them alone. They should do no harm, and as a general principle it's best to avoid destroying evidence. It'd be nice to get some fix for this for alpha2, so I'll commit a fix to do that on Monday, unless we come to a different conclusion before that. +1. Both things sound like a good plan to me. -- 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] Autonomous Transaction is back
On 31 July 2015 23:10, Robert Haas Wrote: On Tue, Jul 28, 2015 at 6:01 AM, Craig Ringer cr...@2ndquadrant.com wrote: That should be practical to special-case by maintaining a list of parent transaction (virtual?) transaction IDs. Attempts to wait on a lock held by any of those should fail immediately. There's no point waiting for the deadlock detector since the outer tx can never progress and commit/rollback to release locks, and it might not be able to see the parent/child relationship from outside the backend doing the nested tx anyway. I think we're going entirely down the wrong path here. Why is it ever useful for a backend's lock requests to conflict with themselves, even with autonomous transactions? That seems like an artifact of somebody else's implementation that we should be happy we don't need to copy. IMHO, since most of the locking are managed at transaction level not backend level and we consider main autonomous transaction to be independent transaction, then practically they may conflict right. It is also right as you said that there is no as such useful use-cases where autonomous transaction conflicts with main (parent) transaction. But we cannot take it for granted as user might make a mistake. So at-least we should have some mechanism to handle this rare case, for which as of now I think throwing error from autonomous transaction as one of the solution. Once error thrown from autonomous transaction, main transaction may continue as it is (or abort main transaction also??). Any other suggestion to handle this will be really helpful. Thanks and Regards, Kumar Rajeev Rastogi -- 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] nodes/*funcs.c inconsistencies
Stephen Frost sfr...@snowman.net writes: Noah, A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic INT/UINT or field order corrections. The non-cosmetic changes involve CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the existing treatments (ignore custom_plans/custom_paths fields; copy/compare cmd string pointer as a scalar) were deliberate, please let me know. Thanks for the review. The change you have is correct for CreatePolicyStmt, at least. I imagine I confused it with polcmd, which is actually just a char. Barring objections, I'll change it to cmd_name after your commit, to reduce the chances of future confusion. Both of you please keep in mind that these cosmetic changes are initdb-forcing, at least if they affect node types that can appear in stored rules. That being the case, it would probably be a good idea to get them done before alpha2, as there may not be a good opportunity afterwards. 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] [sqlsmith] Failed assertion in joinrels.c
On Fri, Jul 31, 2015 at 5:56 PM, Andreas Seltenreich seltenre...@gmx.de wrote: sqlsmith triggered the following assertion in master (c188204). Thanks for writing sqlsmith. It seems like a great tool. I wonder, are you just running the tool with assertions enabled when PostgreSQL is built? If so, it might make sense to make various problems more readily detected. As you may know, Clang has a pretty decent option called AddressSanitizer that can detect memory errors as they occur with an overhead that is not excessive. One might use the following configure arguments when building PostgreSQL to use AddressSanitizer: ./configure CC=clang CFLAGS='-O1 -g -fsanitize=address -fno-omit-frame-pointer -fno-optimize-sibling-calls' --enable-cassert Of course, it remains to be seen if this pays for itself. Apparently the tool has about a 2x overhead [1]. I'm really not sure that you'll find any more bugs this way, but it's certainly possible that you'll find a lot more. Given your success in finding bugs without using AddressSanitizer, introducing it may be premature. [1] http://clang.llvm.org/docs/AddressSanitizer.html#introduction -- 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
[HACKERS] [sqlsmith] subplan variable reference / unassigned NestLoopParams (was: [sqlsmith] Failed assertion in joinrels.c)
Tom Lane writes: Well, I certainly think all of these represent bugs: 3 | ERROR: plan should not reference subplan's variable 2 | ERROR: failed to assign all NestLoopParams to plan nodes These appear to be related. The following query produces the former, but if you replace the very last reference of provider with the literal 'bar', it raises the latter error. select 1 from pg_catalog.pg_shseclabel as rel_09 inner join public.rtest_view2 as rel_32 left join pg_catalog.pg_roles as rel_33 on (rel_32.a = rel_33.rolconnlimit ) on (rel_09.provider = rel_33.rolpassword ) left join pg_catalog.pg_user as rel_35 on (rel_33.rolconfig = rel_35.useconfig ) where ( ((rel_09.provider ~~ 'foo') and (rel_35.usename ~* rel_09.provider))); ,[ FWIW: git bisect run ] | first bad commit: [e83bb10d6dcf05a666d4ada00d9788c7974ad378] | Adjust definition of cheapest_total_path to work better with LATERAL. ` regards, Andreas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Incorrect comment about abbreviated keys
Attached patch fixes this issue. This was missed by 78efd5c1edb59017f06ef96773e64e6539bfbc86 -- Peter Geoghegan From 47ba4759c3d460fa100f0c218b2b06834abfb3f5 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan peter.geoghega...@gmail.com Date: Sun, 2 Aug 2015 02:07:55 -0700 Subject: [PATCH] Fix comment. Commit 78efd5c1edb59017f06ef96773e64e6539bfbc86 overlooked this. --- src/backend/utils/sort/tuplesort.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c index 435041a..1e62a73 100644 --- a/src/backend/utils/sort/tuplesort.c +++ b/src/backend/utils/sort/tuplesort.c @@ -356,9 +356,9 @@ struct Tuplesortstate /* * Additional state for managing abbreviated key sortsupport routines - * (which currently may be used by all cases except the Datum sort case - * and hash index case). Tracks the intervals at which the optimization's - * effectiveness is tested. + * (which currently may be used by all cases except the hash index case). + * Tracks the intervals at which the optimization's effectiveness is + * tested. */ int64 abbrevNext; /* Tuple # at which to next check * applicability */ -- 1.9.1 -- 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] upgrade failure from 9.5 to head
On 2015-08-01 19:13:05 -0400, Noah Misch wrote: On Wed, Jul 29, 2015 at 04:42:55PM -0400, Andrew Dunstan wrote: The next hump is this, in restoring contrib_regression_test_ddl_parse: pg_restore: creating FUNCTION public.text_w_default_in(cstring) pg_restore: [archiver (db)] Error while PROCESSING TOC: pg_restore: [archiver (db)] Error from TOC entry 243; 1255 62534 FUNCTION text_w_default_in(cstring) buildfarm pg_restore: [archiver (db)] could not execute query: ERROR: pg_type OID value not set when in binary upgrade mode Command was: CREATE FUNCTION text_w_default_in(cstring) RETURNS text_w_default LANGUAGE internal STABLE STRICT AS $$texti... Is this worth bothering about, or should I simply remove the database before trying to upgrade? That's a bug. The test_ddl_deparse suite leaves a shell type, which pg_upgrade fails to reproduce. Whether to have pg_upgrade support that or just error out cleanly is another question. There seems little justification to not support shell types. We should also add a shell type to the standard regression testing database, they're weird enough that some increased exposure seems like a good idea. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Minimum tuple threshold to decide last pass of VACUUM
Hi all, Commit 4046e58c (dated of 2001) has introduced the following comment in vacuumlazy.c: + /* If any tuples need to be deleted, perform final vacuum cycle */ + /* XXX put a threshold on min nuber of tuples here? */ + if (vacrelstats-num_dead_tuples 0) In short, we may want to have a reloption to decide if we do or not the last pass of VACUUM or not depending on a given number of remaining tuples. Is this still something we would like to have? Regards, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pageinspect patch, for showing tuple data
Hi!
I've created a patch for pageinspect that allows to see data stored in the
tuple.
This patch has two main purposes:
1. Practical: Make manual DB recovery more simple
2. Educational: Seeing what data is actually stored in tuple, allows to get
better understanding of how does postgres actually works.
This patch adds several new functions, available from SQL queries. All these
functions are based on heap_page_items, but accept slightly different
arguments and has one additional column at the result set:
heap_page_tuples - accepts relation name, and bulkno, and returns usual
heap_page_items set with additional column that contain snapshot of tuple data
area stored in bytea.
heap_page_tuples_attributes - same as heap_page_tuples, but instead of single
tuple data bytea snapshot, it has array of bytea values, that were splitted
into attributes as they would be spitted by nocachegetattr function (I
actually reimplemented this function main algorithm to get this done)
heap_page_tuples_attrs_detoasted - same as heap_page_tuples_attrs, but all
varlen attributes values that were compressed or TOASTed, are replaced with
unTOASTed and uncompressed values.
There is also one strange function: _heap_page_items it is useless for
practical purposes. As heap_page_items it accepts page data as bytea, but also
get a relation name. It tries to apply tuple descriptor of that relation to
that page data.
This would allow you to try to read page data from one table using tuple
descriptor from anther. A strange idea, one should say. But this will allow
you: a) See how wrong data can be interpreted (educational purpose).
b) I have plenty of sanity check while reading parsing that tuple, for this
function I've changed error level from ERROR to WARNING. This function will
allow to write proper tests that all these checks work as they were designed
(I hope to write these tests sooner or later)
I've also added raw tuple data output to original heap_page_items function,
thought I am not sure if it is good idea. I just can add it there so I did it.
May be it would be better to change it back for better backward compatibility.
Attached patched is in almost ready state. It has some formatting issues.
I'd like to hear HACKER's opinion before finishing it and sending to
commitfest.
--
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Companydiff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index aec5258..e4bc1af 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -5,7 +5,7 @@ OBJS = rawpage.o heapfuncs.o btreefuncs.o fsmfuncs.o \
brinfuncs.o ginfuncs.o $(WIN32RES)
EXTENSION = pageinspect
-DATA = pageinspect--1.3.sql pageinspect--1.2--1.3.sql \
+DATA = pageinspect--1.4.sql pageinspect--1.3--1.4.sql pageinspect--1.2--1.3.sql \
pageinspect--1.1--1.2.sql pageinspect--1.0--1.1.sql \
pageinspect--unpackaged--1.0.sql
PGFILEDESC = pageinspect - functions to inspect contents of database pages
diff --git a/contrib/pageinspect/heapfuncs.c b/contrib/pageinspect/heapfuncs.c
index 8d1666c..e296619 100644
--- a/contrib/pageinspect/heapfuncs.c
+++ b/contrib/pageinspect/heapfuncs.c
@@ -29,7 +29,12 @@
#include funcapi.h
#include utils/builtins.h
#include miscadmin.h
+#include utils/array.h
+#include utils/rel.h
+#include catalog/namespace.h
+#include catalog/pg_type.h
+#include rawpage.h
/*
* bits_to_text
@@ -53,6 +58,9 @@ bits_to_text(bits8 *bits, int len)
return str;
}
+Datum heap_page_items_internal(FunctionCallInfo fcinfo, bytea *raw_page, text *relname, int error_level, bool do_detoast);
+void fill_header_data(FuncCallContext *fctx, Datum *values, bool *nulls, bool do_detoast);
+void split_tuple_data(FuncCallContext *fctx, HeapTupleHeader tuphdr, Datum *values, bool *nulls, bool do_detoast);
/*
* heap_page_items
@@ -66,38 +74,98 @@ typedef struct heap_page_items_state
TupleDesc tupd;
Page page;
uint16 offset;
+ TupleDesc page_tuple_desc;
+ int raw_page_size;
+ int error_level;
} heap_page_items_state;
Datum
heap_page_items(PG_FUNCTION_ARGS)
{
bytea *raw_page = PG_GETARG_BYTEA_P(0);
+ text *relname = PG_NARGS()==2 ? PG_GETARG_TEXT_P(1) : NULL;
+
+ /*
+ * Error level is only used while splitting tuple into array of attributes
+ * This is done only for _heap_page_items function. _heap_page_items is intended
+ * for educational and research purposes, so we should change all error checks
+ * to warnings
+ */
+ return heap_page_items_internal(fcinfo, raw_page, relname, WARNING, false);
+}
+
+PG_FUNCTION_INFO_V1(heap_page_tuples);
+Datum
+heap_page_tuples(PG_FUNCTION_ARGS)
+{
+ text *relname = PG_GETARG_TEXT_P(0);
+ uint32 blkno= PG_GETARG_UINT32(1);
+ bytea *raw_page;
+
+ if (SRF_IS_FIRSTCALL())
+ {
+ raw_page = get_raw_page_internal(relname, MAIN_FORKNUM, blkno);
+ }
+ return heap_page_items_internal(fcinfo, raw_page, NULL, ERROR, false);
+}
+
Re: [HACKERS] LWLock deadlock and gdb advice
On 2015-08-02 12:33:06 -0400, Tom Lane wrote: Andres Freund and...@anarazel.de writes: I plan to commit the patch tomorrow, so it's included in alpha2. Please try to commit anything you want in alpha2 *today*. I'd prefer to see some successful buildfarm cycles on such patches before we wrap. Ok, will do that. -- 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] LWLock deadlock and gdb advice
Andres Freund and...@anarazel.de writes: I plan to commit the patch tomorrow, so it's included in alpha2. Please try to commit anything you want in alpha2 *today*. I'd prefer to see some successful buildfarm cycles on such patches before we wrap. 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] MultiXact member wraparound protections are now enabled
On 7/28/15 9:20 AM, Robert Haas wrote: Well, I think that we can eventually downgrade or remove the message once (1) we've actually fixed all of the known multixact bugs and (2) a couple of years have gone by and most people are in the clear. Fair enough. But we should document this better in the future. -- 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] Explanation for intermittent buildfarm pg_upgradecheck failures
I wrote: Further experimentation says that 9.0-9.2 do this in the expected order; so somebody broke it during 9.3. Depressingly enough, the somebody was me. Fixed now. 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
[HACKERS] Explanation for intermittent buildfarm pg_upgradecheck failures
Observe the smoking gun at http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=muledt=2015-08-02%2003%3A30%3A02 to wit this extract from pg_upgrade_server.log: command: /home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/install//home/pg/build-farm-4.15.1/build/HEAD/inst/bin/pg_ctl -w -D /home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/data.old -o stop pg_upgrade_server.log 21 LOG: received fast shutdown request LOG: aborting any active transactions LOG: shutting down waiting for server to shut down.LOG: database system is shut down done server stopped command: /home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/install//home/pg/build-farm-4.15.1/build/HEAD/inst/bin/pg_ctl -w -l pg_upgrade_server.log -D /home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/tmp_check/data -o -p 57832 -b -c synchronous_commit=off -c fsync=off -c full_page_writes=off -c listen_addresses='' -c unix_socket_permissions=0700 -c unix_socket_directories='/home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade' start pg_upgrade_server.log 21 waiting for server to startFATAL: lock file /home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/.s.PGSQL.57832.lock already exists HINT: Is another postmaster (PID 12295) using socket file /home/pg/build-farm-4.15.1/build/HEAD/pgsql.build/src/bin/pg_upgrade/.s.PGSQL.57832? stopped waiting pg_ctl: could not start server Examine the log output. Now, the old postmaster clearly was shut down, so how come the socket lock file still existed? The answer is that what pg_ctl is watching for is for the postmaster.pid file to disappear, and *the postmaster deletes its lock files in the wrong order*. strace'ing HEAD on my own machine shows this sequence of kernel calls during postmaster stop: wait4(-1, 0x723f3cbc, WNOHANG, NULL) = -1 ECHILD (No child processes) stat(backup_label, 0x723f3bd0)= -1 ENOENT (No such file or directory) munmap(0x7fd16e8f8000, 2316)= 0 unlink(/dev/shm/PostgreSQL.1804289383) = 0 semctl(1547862018, 0, IPC_RMID, 0) = 0 semctl(1547894787, 0, IPC_RMID, 0) = 0 semctl(1547927556, 0, IPC_RMID, 0) = 0 semctl(1547960325, 0, IPC_RMID, 0) = 0 semctl(1547993094, 0, IPC_RMID, 0) = 0 semctl(1548025863, 0, IPC_RMID, 0) = 0 semctl(1548058632, 0, IPC_RMID, 0) = 0 semctl(1548091401, 0, IPC_RMID, 0) = 0 shmdt(0x7fd16e8f9000) = 0 munmap(0x7fd165b3c000, 148488192) = 0 shmctl(194314244, IPC_RMID, 0) = 0 unlink(/tmp/.s.PGSQL.5432)= 0 unlink(postmaster.pid)= 0 unlink(/tmp/.s.PGSQL.5432.lock) = 0 exit_group(0) = ? +++ exited with 0 +++ I haven't looked to find out why the unlinks happen in this order, but on a heavily loaded machine, it's certainly possible that the process would lose the CPU after unlink(postmaster.pid), and then a new postmaster could get far enough to see the socket lock file still there. So that would account for low-probability failures in the pg_upgradecheck test, which is exactly what we've been seeing. It seems clear to me that what the exit sequence ought to be is unlink socket, unlink socket lock file, repeat for any additional sockets, then unlink postmaster.pid. Can anyone think of a reason why the current ordering isn't a bug? Another point that's rather obvious from the trace is that at no time do we bother to close the postmaster's TCP socket; it only goes away when the postmaster process dies. So there's a second race condition here: a new postmaster could be unable to bind() to the desired TCP port because the old one still has it open. I think we've seen unexplained failures of the TCP-socket-in-use variety, too. 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] No more libedit?! - openssl plans to switch to APL2
On Sat, Aug 1, 2015 at 11:14 AM, Andres Freund and...@anarazel.de wrote: According to https://www.openssl.org/blog/blog/2015/08/01/cla/ openssl is planning to relicense to the apache license 2.0. While APL2 is not compatible with GLP2 it *is* compatible with GPL3. What's the connection to libedit? -- 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 more libedit?! - openssl plans to switch to APL2
On 2015-08-02 12:34:43 -0400, Robert Haas wrote: On Sat, Aug 1, 2015 at 11:14 AM, Andres Freund and...@anarazel.de wrote: According to https://www.openssl.org/blog/blog/2015/08/01/cla/ openssl is planning to relicense to the apache license 2.0. While APL2 is not compatible with GLP2 it *is* compatible with GPL3. What's the connection to libedit? Some platforms have to use libedit because linking to both libreadline and openssl is of debated legality. GPL prohibits additional restrictions and openssl's license has an advertising clause which is often interpreted violating the additional-restrictions clause. Andres -- 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] Explanation for intermittent buildfarm pg_upgradecheck failures
I wrote: unlink(/tmp/.s.PGSQL.5432)= 0 unlink(postmaster.pid)= 0 unlink(/tmp/.s.PGSQL.5432.lock) = 0 exit_group(0) = ? +++ exited with 0 +++ I haven't looked to find out why the unlinks happen in this order, but on a heavily loaded machine, it's certainly possible that the process would lose the CPU after unlink(postmaster.pid), and then a new postmaster could get far enough to see the socket lock file still there. So that would account for low-probability failures in the pg_upgradecheck test, which is exactly what we've been seeing. Further experimentation says that 9.0-9.2 do this in the expected order; so somebody broke it during 9.3. The lack of a close() on the postmaster socket goes all the way back though. 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] LWLock deadlock and gdb advice
Hi Jeff, Heikki, On 2015-07-31 09:48:28 -0700, Jeff Janes wrote: I had run it for 24 hours, while it usually took less than 8 hours to look up before. I did see it within a few minutes one time when I checked out a new HEAD and then forgot to re-apply your or Heikki's patch. But now I've got the same situation again, after 15 hours, with your patch. This is probably all down to luck. The only differences that I can think of is that I advanced the base to e8e86fbc8b3619da54c, and turned on JJ_vac and set log_autovacuum_min_duration=0. It's quite possible that you hit the remaining race-condition that Heikki was talking about. So that'd make it actually likely to be hit slightly later - but as you say this is just a game of chance. I've attached a version of the patch that should address Heikki's concern. It imo also improves the API and increases debuggability by not having stale variable values in the variables anymore. (also attached is a minor optimization that Heikki has noticed) I plan to commit the patch tomorrow, so it's included in alpha2. Regards, Andres -- 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] LWLock deadlock and gdb advice
On 2015-08-02 17:04:07 +0200, Andres Freund wrote:
I've attached a version of the patch that should address Heikki's
concern. It imo also improves the API and increases debuggability by not
having stale variable values in the variables anymore. (also attached is
a minor optimization that Heikki has noticed)
...
From 17b4e0692ebd91a92448b0abb8cf4438a93a29d6 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Fri, 31 Jul 2015 20:20:43 +0200
Subject: [PATCH 1/2] Fix issues around the variable support in the lwlock
infrastructure.
The lwlock scalability work introduced two race conditions into the
lwlock variable support provided for xlog.c. First, and harmlessly on
most platforms, it set/read the variable without the spinlock in some
places. Secondly, due to the removal of the spinlock, it was possible
that a backend missed changes to the variable's state if it changed in
the wrong moment because checking the lock's state, the variable's state
and the queuing are not protected by a single spinlock acquisition
anymore.
To fix first move resetting the variable's from LWLockAcquireWithVar to
WALInsertLockRelease, via a new function LWLockReleaseClearVar. That
prevents issues around waiting for a variable's value to change when a
new locker has acquired the lock, but not yet set the value. Secondly
re-check that the variable hasn't changed after enqueing, that prevents
the issue that the lock has been released and already re-acquired by the
time the woken up backend checks for the lock's state.
Reported-By: Jeff Janes
Analyzed-By: Heikki Linnakangas
Reviewed-By: Heikki Linnakangas
Discussion: 5592db35.2060...@iki.fi
Backpatch: 9.5, where the lwlock scalability went in
---
src/backend/access/transam/xlog.c | 34 ---
src/backend/storage/lmgr/lwlock.c | 193 +-
src/include/storage/lwlock.h | 2 +-
3 files changed, 129 insertions(+), 100 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1dd31b3..939813e 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1408,9 +1408,7 @@ WALInsertLockAcquire(void)
* The insertingAt value is initially set to 0, as we don't know our
* insert location yet.
*/
- immed = LWLockAcquireWithVar(WALInsertLocks[MyLockNo].l.lock,
- WALInsertLocks[MyLockNo].l.insertingAt,
- 0);
+ immed = LWLockAcquire(WALInsertLocks[MyLockNo].l.lock, LW_EXCLUSIVE);
if (!immed)
{
/*
@@ -1435,26 +1433,28 @@ WALInsertLockAcquireExclusive(void)
int i;
/*
- * When holding all the locks, we only update the last lock's insertingAt
- * indicator. The others are set to 0x, which is higher
- * than any real XLogRecPtr value, to make sure that no-one blocks waiting
- * on those.
+ * When holding all the locks, all but the last lock's insertingAt
+ * indicator is set to 0x, which is higher than any real
+ * XLogRecPtr value, to make sure that no-one blocks waiting on those.
*/
for (i = 0; i NUM_XLOGINSERT_LOCKS - 1; i++)
{
- LWLockAcquireWithVar(WALInsertLocks[i].l.lock,
- WALInsertLocks[i].l.insertingAt,
- PG_UINT64_MAX);
+ LWLockAcquire(WALInsertLocks[i].l.lock, LW_EXCLUSIVE);
+ LWLockUpdateVar(WALInsertLocks[i].l.lock,
+ WALInsertLocks[i].l.insertingAt,
+ PG_UINT64_MAX);
}
- LWLockAcquireWithVar(WALInsertLocks[i].l.lock,
- WALInsertLocks[i].l.insertingAt,
- 0);
+ /* Variable value reset to 0 at release */
+ LWLockAcquire(WALInsertLocks[i].l.lock, LW_EXCLUSIVE);
holdingAllLocks = true;
}
/*
* Release our insertion lock (or locks, if we're holding them all).
+ *
+ * NB: Reset all variables to 0, so they cause LWLockWaitForVar to block the
+ * next time the lock is acquired.
*/
static void
WALInsertLockRelease(void)
@@ -1464,13 +1464,17 @@ WALInsertLockRelease(void)
int i;
for (i = 0; i NUM_XLOGINSERT_LOCKS; i++)
- LWLockRelease(WALInsertLocks[i].l.lock);
+ LWLockReleaseClearVar(WALInsertLocks[i].l.lock,
+ WALInsertLocks[i].l.insertingAt,
+ 0);
holdingAllLocks = false;
}
else
{
- LWLockRelease(WALInsertLocks[MyLockNo].l.lock);
+ LWLockReleaseClearVar(WALInsertLocks[MyLockNo].l.lock,
+ WALInsertLocks[MyLockNo].l.insertingAt,
+ 0);
}
}
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index e5566d1..ae03eb1 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -10,13 +10,15 @@
* locking should be done with the full lock manager --- which depends on
* LWLocks to protect its shared state.
*
- * In addition to exclusive and shared modes, lightweight locks can be used
- * to wait until a variable changes value. The variable is initially set
- * when the lock is acquired with LWLockAcquireWithVar, and can be updated
+ * In addition to exclusive and shared modes, lightweight locks can
Re: [HACKERS] Null pointer passed as source to memcpy() in numeric.c:make_result() and numeric:set_var_from_var()
Piotr Stefaniak postg...@piotr-stefaniak.me writes: these two queries will make the assertions below fail: SELECT STDDEV(0.0); SELECT 0.0 * 0; Fixed, thanks. 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] upgrade failure from 9.5 to head
Andres Freund and...@anarazel.de writes: On 2015-08-01 19:13:05 -0400, Noah Misch wrote: That's a bug. The test_ddl_deparse suite leaves a shell type, which pg_upgrade fails to reproduce. Whether to have pg_upgrade support that or just error out cleanly is another question. There seems little justification to not support shell types. We should also add a shell type to the standard regression testing database, they're weird enough that some increased exposure seems like a good idea. Agreed. I was a bit surprised to find that pg_dump skips shell types, actually. Probably that's a hangover from when create function foo() returns bogus would autocreate a shell type named bogus. In all modern releases, it's fairly hard to accidentally create a shell type, so we should probably assume that the user meant it to be there. 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] nodes/*funcs.c inconsistencies
Noah Misch n...@leadboat.com writes: On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote: I observed these inconsistencies in node support functions: A fresh audit found the attached problems new in 9.5[1]. Many thanks for doing that; I'd had the same checking on my personal to-do list, but now will not need to. 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] [sqlsmith] Failed assertion in joinrels.c
Piotr Stefaniak postg...@piotr-stefaniak.me writes: On 08/01/2015 05:59 PM, Tom Lane wrote: Well, I certainly think all of these represent bugs: 1 | ERROR: could not find pathkey item to sort sqlsmith was able to find these two queries that trigger the error on my machine: Hmm ... I see no error with these queries as of today's HEAD or back-branch tips. I surmise that this was triggered by one of the other recently-fixed bugs, though the connection isn't obvious offhand. 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] nodes/*funcs.c inconsistencies
On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
I observed these inconsistencies in node support functions:
A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic
INT/UINT or field order corrections. The non-cosmetic changes involve
CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the
existing treatments (ignore custom_plans/custom_paths fields; copy/compare
cmd string pointer as a scalar) were deliberate, please let me know.
Thanks,
nm
[1] The _equalCreateEventTrigStmt() cosmetic change is relevant as far back as
9.3, but I won't back-patch it further than the others (9.5).
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 7248440..1c8425d 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -647,6 +647,7 @@ _copyCustomScan(const CustomScan *from)
* copy remainder of node
*/
COPY_SCALAR_FIELD(flags);
+ COPY_NODE_FIELD(custom_plans);
COPY_NODE_FIELD(custom_exprs);
COPY_NODE_FIELD(custom_private);
COPY_NODE_FIELD(custom_scan_tlist);
@@ -1925,9 +1926,9 @@ _copyOnConflictExpr(const OnConflictExpr *from)
COPY_SCALAR_FIELD(action);
COPY_NODE_FIELD(arbiterElems);
COPY_NODE_FIELD(arbiterWhere);
+ COPY_SCALAR_FIELD(constraint);
COPY_NODE_FIELD(onConflictSet);
COPY_NODE_FIELD(onConflictWhere);
- COPY_SCALAR_FIELD(constraint);
COPY_SCALAR_FIELD(exclRelIndex);
COPY_NODE_FIELD(exclRelTlist);
@@ -4082,7 +4083,7 @@ _copyCreatePolicyStmt(const CreatePolicyStmt *from)
COPY_STRING_FIELD(policy_name);
COPY_NODE_FIELD(table);
- COPY_SCALAR_FIELD(cmd);
+ COPY_STRING_FIELD(cmd);
COPY_NODE_FIELD(roles);
COPY_NODE_FIELD(qual);
COPY_NODE_FIELD(with_check);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 6597dbc..1d6c43c 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -759,9 +759,9 @@ _equalOnConflictExpr(const OnConflictExpr *a, const
OnConflictExpr *b)
COMPARE_SCALAR_FIELD(action);
COMPARE_NODE_FIELD(arbiterElems);
COMPARE_NODE_FIELD(arbiterWhere);
+ COMPARE_SCALAR_FIELD(constraint);
COMPARE_NODE_FIELD(onConflictSet);
COMPARE_NODE_FIELD(onConflictWhere);
- COMPARE_SCALAR_FIELD(constraint);
COMPARE_SCALAR_FIELD(exclRelIndex);
COMPARE_NODE_FIELD(exclRelTlist);
@@ -1868,8 +1868,8 @@ _equalCreateEventTrigStmt(const CreateEventTrigStmt *a,
const CreateEventTrigStm
{
COMPARE_STRING_FIELD(trigname);
COMPARE_STRING_FIELD(eventname);
- COMPARE_NODE_FIELD(funcname);
COMPARE_NODE_FIELD(whenclause);
+ COMPARE_NODE_FIELD(funcname);
return true;
}
@@ -2074,7 +2074,7 @@ _equalCreatePolicyStmt(const CreatePolicyStmt *a, const
CreatePolicyStmt *b)
{
COMPARE_STRING_FIELD(policy_name);
COMPARE_NODE_FIELD(table);
- COMPARE_SCALAR_FIELD(cmd);
+ COMPARE_STRING_FIELD(cmd);
COMPARE_NODE_FIELD(roles);
COMPARE_NODE_FIELD(qual);
COMPARE_NODE_FIELD(with_check);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 86f3214..068724e 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -341,7 +341,7 @@ _outModifyTable(StringInfo str, const ModifyTable *node)
WRITE_NODE_FIELD(arbiterIndexes);
WRITE_NODE_FIELD(onConflictSet);
WRITE_NODE_FIELD(onConflictWhere);
- WRITE_INT_FIELD(exclRelRTI);
+ WRITE_UINT_FIELD(exclRelRTI);
WRITE_NODE_FIELD(exclRelTlist);
}
@@ -591,6 +591,7 @@ _outCustomScan(StringInfo str, const CustomScan *node)
_outScanInfo(str, (const Scan *) node);
WRITE_UINT_FIELD(flags);
+ WRITE_NODE_FIELD(custom_plans);
WRITE_NODE_FIELD(custom_exprs);
WRITE_NODE_FIELD(custom_private);
WRITE_NODE_FIELD(custom_scan_tlist);
@@ -1016,7 +1017,7 @@ _outGroupingFunc(StringInfo str, const GroupingFunc *node)
WRITE_NODE_FIELD(args);
WRITE_NODE_FIELD(refs);
WRITE_NODE_FIELD(cols);
- WRITE_INT_FIELD(agglevelsup);
+ WRITE_UINT_FIELD(agglevelsup);
WRITE_LOCATION_FIELD(location);
}
@@ -1532,9 +1533,9 @@ _outOnConflictExpr(StringInfo str, const OnConflictExpr
*node)
WRITE_ENUM_FIELD(action, OnConflictAction);
WRITE_NODE_FIELD(arbiterElems);
WRITE_NODE_FIELD(arbiterWhere);
+ WRITE_OID_FIELD(constraint);
WRITE_NODE_FIELD(onConflictSet);
WRITE_NODE_FIELD(onConflictWhere);
- WRITE_OID_FIELD(constraint);
WRITE_INT_FIELD(exclRelIndex);
WRITE_NODE_FIELD(exclRelTlist);
}
@@ -1674,6 +1675,7 @@ _outCustomPath(StringInfo str, const CustomPath *node)
_outPathInfo(str, (const Path *) node);
WRITE_UINT_FIELD(flags);
+ WRITE_NODE_FIELD(custom_paths);
Re: [HACKERS] nodes/*funcs.c inconsistencies
On Sun, Aug 2, 2015 at 1:28 PM, Noah Misch n...@leadboat.com wrote: A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic INT/UINT or field order corrections. I was responsible for a couple of the cosmetic ones. Sorry about that. It occurs to me that we could do a little more to prevent this automatically. Couldn't we adopt AssertVariableIsOfType()/AssertVariableIsOfTypeMacro() to macros like READ_UINT_FIELD()? I'm surprised that this stuff was only ever used for logical decoding infrastructure so far. -- 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] nodes/*funcs.c inconsistencies
On Sun, Aug 2, 2015 at 3:07 PM, Peter Geoghegan p...@heroku.com wrote: I'm surprised that this stuff was only ever used for logical decoding infrastructure so far. On second thought, having tried it, one reason is that that breaks things that are considered legitimate for historical reasons. For example, AttrNumber is often used with READ_INT_FIELD(), which is an int16. Whether or not it's worth fixing that by introducing a READ_ATTRNUM_FIELD() (and so on) is not obvious to me. -- 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] upgrade failure from 9.5 to head
On 08/02/2015 04:00 PM, Tom Lane wrote: Andres Freund and...@anarazel.de writes: On 2015-08-01 19:13:05 -0400, Noah Misch wrote: That's a bug. The test_ddl_deparse suite leaves a shell type, which pg_upgrade fails to reproduce. Whether to have pg_upgrade support that or just error out cleanly is another question. There seems little justification to not support shell types. We should also add a shell type to the standard regression testing database, they're weird enough that some increased exposure seems like a good idea. Agreed. I was a bit surprised to find that pg_dump skips shell types, actually. Probably that's a hangover from when create function foo() returns bogus would autocreate a shell type named bogus. In all modern releases, it's fairly hard to accidentally create a shell type, so we should probably assume that the user meant it to be there. I'm fine with fixing it, but what's the actual use case for a long lived shell type? 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] Sharing aggregate states between different aggregate functions
On 29 July 2015 at 03:45, Heikki Linnakangas hlinn...@iki.fi wrote: On 07/28/2015 04:14 AM, David Rowley wrote: On 27 July 2015 at 20:11, Heikki Linnakangas hlinn...@iki.fi wrote: On 07/27/2015 08:34 AM, David Rowley wrote: In this function I also wasn't quite sure if it was with comparing both non-NULL INITCOND's here. I believe my code comments may slightly contradict what the code actually does, as the comments talk about them having to match, but the code just bails if any are non-NULL. The reason I didn't check them was because it seems inevitable that some duplicate work needs to be done when setting up the INITCOND. Perhaps it's worth it? It would be nice to handle non-NULL initconds. I think you'll have to check that the input function isn't volatile. Or perhaps just call the input function, and check that the resulting Datum is byte-per-byte identical, although that might be awkward to do with the current code structure. I've not done anything with this. I'd not thought of an input function being volatile before, but I guess it's possible, which makes me a bit scared that we could be treading on ground we shouldn't be. I know it's more of an output function thing than an input function thing, but a GUC like extra_float_digits could cause problems here. Yeah, a volatile input function seems highly unlikely, but who knows. BTW, we're also not checking if the transition or final functions are volatile. But that was the same before this patch too. It sure would be nice to support the built-in float aggregates, so I took a stab at this. I heavily restructured the code again, so that there are now two separate steps. First, we check for any identical Aggrefs that could be shared. If that fails, we proceed to the permission checks, look up the transition function and build the initial datum. And then we call another function that tries to find an existing, compatible per-trans structure. I think this actually looks better than before, and checking for identical init values is now easy. This does lose one optimization: if there are two aggregates with identical transition functions and final functions, they are not merged into a single per-Agg struct. They still share the same per-Trans struct, though, and I think that's enough. How does the attached patch look to you? The comments still need some cleanup, in particular, the explanations of the different scenarios don't belong where they are anymore. I've read over the patch and you've managed to implement the init value checking much more cleanly than I had imagined it to be. I like the 2 stage checking. Attached is a delta patched which is based on sharing_aggstate-heikki-2.patch to fix up the out-dated comments and also a few more test scenarios which test the sharing works with matching INITCOND and that it does not when they don't match. What do you think? BTW, the permission checks were not correct before. You cannot skip the check on the transition function when you're sharing the per-trans state. We check that the aggregate's owner has permission to execute the transition function, and the previous aggregate whose state value we're sharing might have different owner. oops, thank for noticing that and fixing. Regards David Rowley -- David Rowley http://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Training Services sharing_aggstate-heikki-2_delta1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] upgrade failure from 9.5 to head
On 2015-08-02 19:06:49 -0400, Andrew Dunstan wrote: I'm fine with fixing it, but what's the actual use case for a long lived shell type? The use-case imo is that we shouldn't just break if somebody did something stupid or was busy creating a new type while a scheduled backup ran. Andres -- 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] nodes/*funcs.c inconsistencies
Noah, * Noah Misch (n...@leadboat.com) wrote: On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote: I observed these inconsistencies in node support functions: A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic INT/UINT or field order corrections. The non-cosmetic changes involve CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the existing treatments (ignore custom_plans/custom_paths fields; copy/compare cmd string pointer as a scalar) were deliberate, please let me know. Thanks for the review. The change you have is correct for CreatePolicyStmt, at least. I imagine I confused it with polcmd, which is actually just a char. Barring objections, I'll change it to cmd_name after your commit, to reduce the chances of future confusion. Thanks again! Stephen signature.asc Description: Digital signature
Re: [HACKERS] nodes/*funcs.c inconsistencies
On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote: I observed these inconsistencies in node support functions: A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic INT/UINT or field order corrections. The non-cosmetic changes involve CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the existing treatments (ignore custom_plans/custom_paths fields; copy/compare cmd string pointer as a scalar) were deliberate, please let me know. Thanks for your works. I also noticed one other inconsistent point; _outMergeJoin() dumps mergeNullsFirst[] array but it does not use booltostr() macro. Best regards, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei kai...@ak.jp.nec.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] Explanation for intermittent buildfarm pg_upgradecheck failures
On Mon, Aug 3, 2015 at 1:30 AM, Tom Lane t...@sss.pgh.pa.us wrote: I haven't looked to find out why the unlinks happen in this order, but on a heavily loaded machine, it's certainly possible that the process would lose the CPU after unlink(postmaster.pid), and then a new postmaster could get far enough to see the socket lock file still there. So that would account for low-probability failures in the pg_upgradecheck test, which is exactly what we've been seeing. Oh... This may explain the different failures seen with TAP tests on hamster, and axolotl with pg_upgrade as well. It is rather easy to get them heavily loaded. -- 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] upgrade failure from 9.5 to head
* Andres Freund (and...@anarazel.de) wrote: On 2015-08-01 19:13:05 -0400, Noah Misch wrote: On Wed, Jul 29, 2015 at 04:42:55PM -0400, Andrew Dunstan wrote: The next hump is this, in restoring contrib_regression_test_ddl_parse: pg_restore: creating FUNCTION public.text_w_default_in(cstring) pg_restore: [archiver (db)] Error while PROCESSING TOC: pg_restore: [archiver (db)] Error from TOC entry 243; 1255 62534 FUNCTION text_w_default_in(cstring) buildfarm pg_restore: [archiver (db)] could not execute query: ERROR: pg_type OID value not set when in binary upgrade mode Command was: CREATE FUNCTION text_w_default_in(cstring) RETURNS text_w_default LANGUAGE internal STABLE STRICT AS $$texti... Is this worth bothering about, or should I simply remove the database before trying to upgrade? That's a bug. The test_ddl_deparse suite leaves a shell type, which pg_upgrade fails to reproduce. Whether to have pg_upgrade support that or just error out cleanly is another question. There seems little justification to not support shell types. We should also add a shell type to the standard regression testing database, they're weird enough that some increased exposure seems like a good idea. +1. I was doing testing the other day and ran into the pg_dump doesn't support shell types issue and it was annoyingly confusing. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] nodes/*funcs.c inconsistencies
On Mon, Aug 03, 2015 at 01:32:10AM +, Kouhei Kaigai wrote: On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote: I observed these inconsistencies in node support functions: A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic INT/UINT or field order corrections. The non-cosmetic changes involve CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the existing treatments (ignore custom_plans/custom_paths fields; copy/compare cmd string pointer as a scalar) were deliberate, please let me know. Thanks for your works. I also noticed one other inconsistent point; _outMergeJoin() dumps mergeNullsFirst[] array but it does not use booltostr() macro. Good catch. All supported branches work that way, and it's not wrong. I recommend keeping it unchanged. -- 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 : Allow toast tables to be moved to a different tablespace
On 07/15/2015 09:30 PM, Robert Haas wrote: On Tue, Jul 14, 2015 at 5:57 PM, Jim Nasby jim.na...@bluetreble.com wrote: On 7/7/15 7:07 AM, Andres Freund wrote: On 2015-07-03 18:03:58 -0400, Tom Lane wrote: I have just looked through this thread, and TBH I think we should reject this patch altogether --- not RWF, but no we don't want this. The use-case remains hypothetical: no performance numbers showing a real-world benefit have been exhibited AFAICS. It's not that hard to imagine a performance benefit though? If the toasted column is accessed infrequently/just after filtering on other columns (not uncommon) it could very well be beneficial to put the main table on fast storage (SSD) and the toast table on slow storage (spinning rust). I've seen humoungous toast tables that are barely ever read for tables that are constantly a couple times in the field. +1. I know of one case where the heap was ~8GB and TOAST was over 400GB. Yeah, I think that's a good argument for this. I have to admit that I'm a bit fatigued by this thread - it started out as a learn PostgreSQL project, and we iterated through a few design problems that made me kind of worried about what sort of state the patch was in - and now this patch is more than 4 years old. But if some committer still has the energy to go through it in detail and make sure that all of the problems have been fixed and that the patch is, as Andreas says, in good shape, then I don't see why we shouldn't take it. I personally think the patch is in a decent shape, and a worthwhile feature. I agree though with Tom's objections about the pg_dump code. I do not have enough time or interest right now to fix up this patch (this is not a feature I personally have a lot of interest in), but if someone else wishes to I do not think it would be too much work. Andreas -- 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] Improving test coverage of extensions with pg_dump
On Thu, Jul 30, 2015 at 5:35 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
Note as well that I will be fine with any decision taken by the
committer who picks up this, this test case is useful by itself in any
case.
And I just recalled that I actually did no tests for this thing on
Windows. As this uses the TAP facility, I think that it makes most
sense to run it with tapcheck instead of modulescheck in vcregress.pl
because of its dependency with IPC::Run. The compilation with MSVC is
fixed as well.
--
Michael
From a56e2ed296533e30bff47df11f68f0f415ae8a3f Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Sun, 2 Aug 2015 19:11:58 -0700
Subject: [PATCH] Add TAP test for pg_dump checking data dump of extension
tables
The test added checks the data dump ordering of tables added in an extension
linked among them with foreign keys. In order to do that, a test extension in
the set of TAP tests of pg_dump, combined with a TAP test script making use
of it.
---
src/test/modules/Makefile | 1 +
src/test/modules/tables_fk/.gitignore | 1 +
src/test/modules/tables_fk/Makefile | 24 +
src/test/modules/tables_fk/README | 5
src/test/modules/tables_fk/t/001_dump_test.pl | 39 +++
src/test/modules/tables_fk/tables_fk--1.0.sql | 20 ++
src/test/modules/tables_fk/tables_fk.control | 5
src/tools/msvc/Mkvcbuild.pm | 2 +-
src/tools/msvc/vcregress.pl | 3 +++
9 files changed, 99 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/tables_fk/.gitignore
create mode 100644 src/test/modules/tables_fk/Makefile
create mode 100644 src/test/modules/tables_fk/README
create mode 100644 src/test/modules/tables_fk/t/001_dump_test.pl
create mode 100644 src/test/modules/tables_fk/tables_fk--1.0.sql
create mode 100644 src/test/modules/tables_fk/tables_fk.control
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 8213e23..683e9cb 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -7,6 +7,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = \
commit_ts \
dummy_seclabel \
+ tables_fk \
test_ddl_deparse \
test_parser \
test_rls_hooks \
diff --git a/src/test/modules/tables_fk/.gitignore b/src/test/modules/tables_fk/.gitignore
new file mode 100644
index 000..b6a2a01
--- /dev/null
+++ b/src/test/modules/tables_fk/.gitignore
@@ -0,0 +1 @@
+/tmp_check/
diff --git a/src/test/modules/tables_fk/Makefile b/src/test/modules/tables_fk/Makefile
new file mode 100644
index 000..d018517
--- /dev/null
+++ b/src/test/modules/tables_fk/Makefile
@@ -0,0 +1,24 @@
+# src/test/modules/tables_fk/Makefile
+
+EXTENSION = tables_fk
+DATA = tables_fk--1.0.sql
+PGFILEDESC = tables_fk - set of dumpable tables linked by foreign keys
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/tables_fk
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: all
+ $(prove_check)
+
+installcheck: install
+ $(prove_installcheck)
+
+temp-install: EXTRA_INSTALL=src/test/modules/tables_fk
diff --git a/src/test/modules/tables_fk/README b/src/test/modules/tables_fk/README
new file mode 100644
index 000..049b75c
--- /dev/null
+++ b/src/test/modules/tables_fk/README
@@ -0,0 +1,5 @@
+tables_fk
+=
+
+Simple module used to check data dump ordering of pg_dump with tables
+linked with foreign keys using TAP tests.
diff --git a/src/test/modules/tables_fk/t/001_dump_test.pl b/src/test/modules/tables_fk/t/001_dump_test.pl
new file mode 100644
index 000..9d3d5ff
--- /dev/null
+++ b/src/test/modules/tables_fk/t/001_dump_test.pl
@@ -0,0 +1,39 @@
+use strict;
+use warnings;
+use TestLib;
+use Test::More tests = 4;
+
+my $tempdir = tempdir;
+
+start_test_server $tempdir;
+
+psql 'postgres', 'CREATE EXTENSION tables_fk';
+
+# Insert some data before running the dump, this is needed to check
+# consistent data dump of tables with foreign key dependencies
+psql 'postgres', 'INSERT INTO cc_tab_fkey VALUES (1)';
+psql 'postgres', 'INSERT INTO bb_tab_fkey VALUES (1)';
+psql 'postgres', 'INSERT INTO aa_tab_fkey VALUES (1)';
+
+# Create a table depending on a FK defined in the extension
+psql 'postgres', 'CREATE TABLE dd_tab_fkey (id int REFERENCES bb_tab_fkey(id))';
+psql 'postgres', 'INSERT INTO dd_tab_fkey VALUES (1)';
+
+# Take a dump then re-deploy it to a new database
+command_ok(['pg_dump', '-d', 'postgres', '-f', $tempdir/dump.sql],
+ 'taken dump with installed extension');
+command_ok(['createdb', 'foobar1'], 'created new database to redeploy dump');
+command_ok(['psql', '--set=ON_ERROR_STOP=on', '-d', 'foobar1', '-f',
+ $tempdir/dump.sql], 'dump restored');
+
+# And check its content
+my $result = psql 'foobar1',
+ q{SELECT id
