Re: [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
Hi, On Sat, May 10, 2008 at 6:11 AM, Alex Hunsaker [EMAIL PROTECTED] wrote: On Fri, May 9, 2008 at 5:37 PM, Tom Lane [EMAIL PROTECTED] wrote: Alex Hunsaker [EMAIL PROTECTED] writes: [ patch to change inherited-check-constraint behavior ] Applied after rather heavy editorializations. You didn't do very well on getting it to work in multiple-inheritance scenarios, such as create table p (f1 int check (f10)); create table c1 (f2 int) inherits (p); create table c2 (f3 int) inherits (p); create table cc () inherits (c1,c2); Here the same constraint is multiply inherited. The base case as above worked okay, but adding the constraint to an existing inheritance tree via ALTER TABLE, not so much. Ouch. Ok Ill (obviously) review what you committed so I can do a lot better next time. Thanks for muddling through it! Ouchie indeed! I'm not sure if we ought to try to back-patch that --- it'd be a behavioral change with non-obvious implications. In the back branches, ADD CHECK followed by DROP CONSTRAINT will end up not deleting the child-table constraints, which is probably a bug but I wouldn't be surprised if applications were depending on the behavior. Given the lack complaints it does not seem worth a back patch IMHO. Yeah, same IMHO. I do hope we have covered things properly for inherited check constraints by now. One minor thing that myself and Alex discussed was the usage of child tables in tablecmds.c, especially in error messages. Again English is not my native language, but shouldn't that be worded as children tables? Admittedly even this does not sound any better than child tables though :). It is nit-picking really, but I can submit a cleanup patch to reword this if the list thinks so.. Regards, Nikhils -- EnterpriseDB http://www.enterprisedb.com
[PATCHES] pg_dump lock timeout
Attached is a patch to add a commandline option to pg_dump to limit how long pg_dump will wait for locks during startup. The intent of this patch is to allow pg_dump to fail if a table lock cannot be taken in a reasonable time. This allows the caller of pg_dump to retry or otherwise correct the situation, without having locks held for long periods, and without pg_dump having a long window during which catalog changes can occur. It works by setting statement_timeout to the user specified delay during the startup phase where it is taking access share locks on all the tables. Once all the locks are taken, it sets statement_timeout back to the default. If a lock table statement times out, the dump fails with the statement timed out error. The orginal motivation was a client who runs heavy batch workloads and uses truncate table and other DML in long transactions. This has created some unhappy interaction scenarios with pg_dump: - pg_dump ends up waiting hours on a DML table lock that is part of a long transaction. Once the lock is released, pg_dump runs only to find some table later in the list has been dropped. So pg_dump fails. - pg_dump waits on a lock while holding access share locks on most of the tables. Other processes that want to do DML wait on pg_dump. After a while, large parts of the application are blocked while pg_dump waits on locks. Eventually the operations staff notice that pg_dump is blocking production and kill the dump. Please have a look and consider it for merging. Thanks -dg -- David Gould If simplicity worked, the world would be overrun with insects. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] pg_dump lock timeout
On Sun, May 11, 2008 at 04:30:47AM -0700, daveg wrote:
Attached is a patch to add a commandline option to pg_dump to limit how long
pg_dump will wait for locks during startup.
Ooops, really attached this time.
-dg
--
David Gould [EMAIL PROTECTED] 510 536 1443510 282 0869
If simplicity worked, the world would be overrun with insects.
*** pgsql/src/bin/pg_dump/pg_dump.c.orig2008-05-11 03:23:06.0
-0700
--- pgsql/src/bin/pg_dump/pg_dump.c 2008-05-11 03:44:58.0 -0700
***
*** 71,76
--- 71,77
bool schemaOnly;
bool dataOnly;
bool aclsSkip;
+ const char*lockWaitTimeout;
/* subquery used to convert user ID (eg, datdba) to user name */
static const char *username_subquery;
***
*** 238,243
--- 239,245
{column-inserts, no_argument, NULL, 'D'},
{host, required_argument, NULL, 'h'},
{ignore-version, no_argument, NULL, 'i'},
+ {lock-wait-timeout, required_argument, NULL, 'l'},
{no-reconnect, no_argument, NULL, 'R'},
{oids, no_argument, NULL, 'o'},
{no-owner, no_argument, NULL, 'O'},
***
*** 278,283
--- 280,286
strcpy(g_opaque_type, opaque);
dataOnly = schemaOnly = dumpInserts = attrNames = false;
+ lockWaitTimeout = NULL;
progname = get_progname(argv[0]);
***
*** 299,305
}
}
! while ((c = getopt_long(argc, argv,
abcCdDE:f:F:h:in:N:oOp:RsS:t:T:U:vWxX:Z:,
long_options,
optindex)) != -1)
{
switch (c)
--- 302,308
}
}
! while ((c = getopt_long(argc, argv,
abcCdDE:f:F:h:il:n:N:oOp:RsS:t:T:U:vWxX:Z:,
long_options,
optindex)) != -1)
{
switch (c)
***
*** 350,355
--- 353,362
/* ignored, deprecated option */
break;
+ case 'l': /* lock wait time */
+ lockWaitTimeout = optarg;
+ break;
+
case 'n': /* include schema(s) */
simple_string_list_append(schema_include_patterns, optarg);
include_everything = false;
***
*** 755,760
--- 762,769
printf(_(\nGeneral options:\n));
printf(_( -f, --file=FILENAME output file name\n));
printf(_( -F, --format=c|t|p output file format (custom, tar,
plain text)\n));
+ printf(_( -l, --lock-wait-timeout=DELAY\n
+timeout and fail after
delay waiting for a table share lock\n));
printf(_( -v, --verboseverbose mode\n));
printf(_( -Z, --compress=0-9 compression level for compressed
formats\n));
printf(_( --help show this help, then exit\n));
***
*** 3191,3196
--- 3200,3213
i_reltablespace = PQfnumber(res, reltablespace);
i_reloptions = PQfnumber(res, reloptions);
+ if (lockWaitTimeout)
+ {
+ /* Abandon the dump instead of waiting forever for a table lock
*/
+ resetPQExpBuffer(lockquery);
+ appendPQExpBuffer(lockquery, SET statement_timeout = );
+ appendStringLiteralConn(lockquery, lockWaitTimeout, g_conn);
+ do_sql_command(g_conn, lockquery-data);
+ }
for (i = 0; i ntups; i++)
{
tblinfo[i].dobj.objType = DO_TABLE;
***
*** 3259,3264
--- 3276,3285
tblinfo[i].dobj.name);
}
+ if (lockWaitTimeout)
+ {
+ do_sql_command(g_conn, SET statement_timeout = default);
+ }
PQclear(res);
/*
*** pgsql/doc/src/sgml/ref/pg_dump.sgml.orig2008-05-11 03:38:05.0
-0700
--- pgsql/doc/src/sgml/ref/pg_dump.sgml 2008-05-11 03:38:56.0 -0700
***
*** 302,307
--- 302,320
/varlistentry
varlistentry
+ termoption-l replaceable
class=parameterwait_time/replaceable/option/term
+ termoption--lock-wait-timeout=replaceable
class=parameterwait_time/replaceable/option/term
+ listitem
+para
+ Do not wait forever for table locks at the start of the dump. Instead
+ time out and abandon the dump if unable to lock a table within the
+ specified wait time. The wait time is specified with the same formats
+ as accepted for intervals by the SET command.
+/para
+ /listitem
+ /varlistentry
+
+
Re: [PATCHES] Database owner installable modules patch
On Sat, May 10, 2008 at 11:02 AM, Bruce Momjian [EMAIL PROTECTED] wrote: Where are we on this? I haven't had time to do any work since the original patch. That patch was fairly basic - it just ran install / uninstall scripts and created catalog entries, and introduced some slightly exotic syntax to do it (INSTALL/UNINSTALL vs CREATE/DROP). The next version is intended to handle dependencies, which should make uninstallation straight forward for most cases. I was intending to revert the syntax creativity and make the commands CREATE/DROP too. I'll get a bit of time to look at both this and the enum patch this week. Cheers Tom -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [EMAIL PROTECTED]: Re: [BUGS] Problem identifying constraints which should not be inherited]
Nikhils [EMAIL PROTECTED] writes: ... One minor thing that myself and Alex discussed was the usage of child tables in tablecmds.c, especially in error messages. Again English is not my native language, but shouldn't that be worded as children tables? Admittedly even this does not sound any better than child tables though :). No, child tables sounds better to me. English doesn't usually pluralize adjectives. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Snapshot management, final
Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: Also, I think that the whole snapshot-sharing mechanism is not working as intended except for the serializable case; otherwise sequences like x = RegisterSnapshot(GetTransactionSnapshot()); y = RegisterSnapshot(GetTransactionSnapshot()); will result in x and y being separate copies. Or are you assuming that this just isn't worth optimizing? It's not that I don't think it's worth optimizing, but I think it's a bit away from the scope of this patch. The problem here is how to notice that two consecutive GetTransactionSnapshot calls should really return different snapshots, considering that shared state may change in between. Perhaps there's an easy way to optimize that; I don't know. Yeah, in general you could only optimize it if no other backend had changed state, and there doesn't seem any real simple way to know that. Maybe we could teach GetSnapshotData to test for it but it's a bit doubtful that it's worth the cycles. I'm fine with leaving this as-is. I have a few other gripes though: The UnregisterSnapshot call at line 631 of indexcmds.c is definitely too early: the snapshot is touched in the very next statement. I'd be inclined to move it down to right after the Pop at line 696; there's no point in unregistering till you pop anyway. In FreeQueryDesc, the unregister calls should be after the Assert. Drop this comment fragment in plancache.c, it's not relevant anymore: * Having to !* replan is an unusual case, and it seems a really bad idea for !* RevalidateCachedPlan to affect the snapshot only in unusual !* cases. !*/ s_level and as_level fields must be int not uint32, because they are being compared to nesting levels that are declared as int. You risk getting the wrong comparison semantics. (Not that it should ever matter in this code, but mixing signed and unsigned arithmetic is just bad form.) Why Assert(snap-satisfies == HeapTupleSatisfiesMVCC) in PushActiveSnapshot and RegisterSnapshot? AFAICT this code will work fine on non-MVCC snapshots. Seems a bit odd that RegisterSnapshot and PushActiveSnapshot do their palloc's in opposite orders. I think making the list elt first is probably better; in either case, if the second palloc fails then you're gonna leak the first one, so it's better to make the smaller allocation first. Shouldn't UnregisterSnapshot insist that s_level be equal to current xact nest level? AtSubCommit_Snapshot can leave us with multiple RegdSnapshotElt's for the same snap and s_level, which seems a bit bogus. I don't think the unregister code will go wrong in its current form, but you at least need some comments about the invariants that are expected to hold for the list data structure. Example: the code depends on the assumption that elements are in nonincreasing s_level order, but that's nowhere stated. AtSubAbort_Snapshot has Assert(tofree-s_snap-active_count == 0) which seems wrong: couldn't the snap be active in an outer subxact? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] options for RAISE statement
Pavel Stehule [EMAIL PROTECTED] writes: I am sending enhanced version of original patch. Hmm ... this patch seems to have been generated against something significantly different from HEAD ... was that intentional? patching file plpgsql.sgml Hunk #1 succeeded at 2102 (offset -82 lines). Hunk #3 succeeded at 2167 (offset -82 lines). Hunk #5 succeeded at 2807 (offset -82 lines). patching file gram.y Hunk #1 succeeded at 52 (offset -1 lines). Hunk #2 succeeded at 141 with fuzz 2 (offset -2 lines). Hunk #3 succeeded at 1262 (offset -45 lines). Hunk #4 succeeded at 1314 (offset -2 lines). Hunk #5 succeeded at 1279 (offset -45 lines). Hunk #6 succeeded at 1646 (offset -2 lines). Hunk #7 succeeded at 2703 (offset -144 lines). patching file pl_comp.c Hunk #1 succeeded at 1750 (offset -1 lines). patching file pl_exec.c Hunk #1 succeeded at 2270 (offset -97 lines). patching file pl_funcs.c Hunk #1 succeeded at 1012 (offset -43 lines). patching file plpgsql.h Hunk #1 succeeded at 120 (offset -1 lines). Hunk #2 succeeded at 554 (offset -18 lines). Hunk #3 succeeded at 808 (offset -1 lines). patching file plpgsql.out Hunk #1 FAILED at 3385. 1 out of 1 hunk FAILED -- saving rejects to file plpgsql.out.rej patching file plpgsql.sql Hunk #1 FAILED at 2735. 1 out of 1 hunk FAILED -- saving rejects to file plpgsql.sql.rej regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Snapshot management, final
Tom Lane wrote: I'm revising the patch; this comment is flawed though: Shouldn't UnregisterSnapshot insist that s_level be equal to current xact nest level? It can't check that; consider begin; savepoint foo; declare cur cursor for select (1), (2), (3); savepoint bar; close cur; commit; Thanks again for the review. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Snapshot management, final
Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: Shouldn't UnregisterSnapshot insist that s_level be equal to current xact nest level? It can't check that; consider begin; savepoint foo; declare cur cursor for select (1), (2), (3); savepoint bar; close cur; commit; Hmm ... but that close can't unregister the snapshot immediately, because you'd lose if the 2nd savepoint gets rolled back, no? Is the handling of this case even correct at the moment? ISTM correct handling of this example would require that the close not really discard the snap until commit. Then, given proper ordering of the cleanup operations at commit, you might be able to still have the cross-check about s_level in UnregisterSnapshot. (IOW, maybe having snapshot cleanup be late in the commit sequence wasn't such a good choice...) regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Snapshot management, final
Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: Shouldn't UnregisterSnapshot insist that s_level be equal to current xact nest level? It can't check that; consider begin; savepoint foo; declare cur cursor for select (1), (2), (3); savepoint bar; close cur; commit; Hmm ... but that close can't unregister the snapshot immediately, because you'd lose if the 2nd savepoint gets rolled back, no? Is the handling of this case even correct at the moment? No, CLOSE is not rolled back: alvherre=# begin; BEGIN alvherre=# savepoint foo; SAVEPOINT alvherre=# declare cur cursor for select (1), (2), (3); DECLARE CURSOR alvherre=# savepoint bar; SAVEPOINT alvherre=# close cur; CLOSE CURSOR alvherre=# rollback to bar; ROLLBACK alvherre=# fetch all from cur; ERREUR: le curseur « cur » n'existe pas Maybe this is possible to fix, but again I think it's outside the scope of this patch. ISTM correct handling of this example would require that the close not really discard the snap until commit. Then, given proper ordering of the cleanup operations at commit, you might be able to still have the cross-check about s_level in UnregisterSnapshot. (IOW, maybe having snapshot cleanup be late in the commit sequence wasn't such a good choice...) Right -- I'll move them earlier. I don't think it's trivial to fix the un-rollback-ability of CLOSE however. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Snapshot management, final
Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: Hmm ... but that close can't unregister the snapshot immediately, because you'd lose if the 2nd savepoint gets rolled back, no? Is the handling of this case even correct at the moment? No, CLOSE is not rolled back: ... Maybe this is possible to fix, but again I think it's outside the scope of this patch. I'd forgotten that ... seems a bit bogus, and it's certainly not documented on the CLOSE reference page. ISTM correct handling of this example would require that the close not really discard the snap until commit. Then, given proper ordering of the cleanup operations at commit, you might be able to still have the cross-check about s_level in UnregisterSnapshot. (IOW, maybe having snapshot cleanup be late in the commit sequence wasn't such a good choice...) Right -- I'll move them earlier. Well, without a clear idea of where to place them instead, you might as well leave it alone for the moment. I'd like to see this revisited sometime though. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
