Re: [HACKERS] pg_terminate_backend for same-role
On Thu, Mar 15, 2012 at 10:45 PM, Tom Lane t...@sss.pgh.pa.us wrote: Daniel Farina dan...@heroku.com writes: On Thu, Mar 15, 2012 at 10:33 PM, Tom Lane t...@sss.pgh.pa.us wrote: But actually I don't see what you hope to gain from such a change, even if it can be made to work. Anyone who can do kill(SIGINT) can do kill(SIGKILL), say --- so you have to be able to trust the signal sender. What's the point of not trusting it to verify the client identity? No longer true with pg_cancel_backend not-by-superuser, no? No. That doesn't affect the above argument in the least. And in fact if there's any question whatsoever as to whether unprivileged cross-backend signals are secure, they are not going in in the first place. Okay, well, I believe there is a race in handling common administrative signals that *might* possibly matter. In the past, pg_cancel_backend was superuser only, which is a lot like saying only available to people who can be the postgres user and run kill. The cancellation packet is handled via first checking the other backend's BackendKey and then SIGINTing it, leaving only the most narrow possibility for a misdirected SIGINT. But it really is unfortunate that I, a user, run a query or have a problematic connection of my own role and just want the thing to stop, but I can't do anything about it without superuser. In recognition of that, pg_cancel_backend now can operate on backends owned by the same user (once again, checked before the signal is processed by the receiver, just like with the cancellation packet). There was some angst (but I'm not sure about how specific or uniform) to extend such signaling power to pg_terminate_backend, and the only objection I can think of is there is this race, or so it would seem to me. Maybe it's too small to care, in which case we can just extend the same policy to pg_terminate_backend, or maybe it's not, in which case we could get rid of any signaling race conditions. The only hypothetical person who would be happy with the current situation, if characterized correctly, would be one who thinks that pid-race on SIGINT from non-superusers (long has been true in the form of the cancellation packet) is okay but on SIGTERM such a race is not. -- fdr -- 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_upgrade and statistics
On Thu, Mar 15, 2012 at 8:48 PM, Alvaro Herrera alvhe...@commandprompt.com What Peter proposed seems to me pretty reasonable, in the sense that it should be possible to come up with a function that creates some text representation of whatever is in pg_statistic, and another function to load that data into the new catalog(s). There's no need to keep pg_statistic binary-compatible, or even continue to have only pg_statistic (IIRC Zoltan/Hans-Jurgen patch for cross-column stats adds a new catalog, pg_statistic2 or similar). If the upgrade target release has room for more/improved stats, that's fine -- they'll be unused after loading the stats from the dump. And the old stats can be reacommodated, if necessary. I have been reading up on selectivity estimation research for the last few days. I must say that I also think that having a text representation as an intermediate won't create a huge maintenance burden. The basic concepts that are there are pretty solid. Conceptually MCV's and histograms continue to be essential even with the more complex approaches. Trying to maintain binary compatibility is probably a bad idea, as Tom noted with the array selectivity patch - encoding of the information could be better. But given a textual format it won't be too hard to just massage the data to the new format. Making it possible to dump and load stats has the additional bonus of enabling more experimentation with custom stats collectors. One could easily prototype the stats collection with R, scipy, etc. Of course the proof will be in the pudding. Re, the patch, current posted WIP cross-col patch doesn't create a new catalog,. It repurposes the stat slots mechanism to store multiple dimensions. But I'll most likely rewrite it to use a separate catalog because the storage requirements are rather different. I'll post a proposal in the appropriate thread when I have decently clear idea how this should work. One thing that seems clear is that multi-dimensional histograms will want this mechanism even more, optimal histogram construction is NP-hard in the multi-dimensional case and so people will want to try different algorithms, or make different tradeoffs on effort spent on constructing the histogram. Or even build one by hand. Cheers, Ants Aasma -- 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] [v9.2] Add GUC sepgsql.client_label
On 2012-03-15 21:45, Robert Haas wrote: On Wed, Mar 14, 2012 at 11:10 AM, Kohei KaiGaikai...@kaigai.gr.jp wrote: If it is ready to commit, please remember the credit to Yeb's volunteer on this patch. Done. In the patch with copy-editing documentation following that commit, at in at their option, s/in// ? Also 'rather than .. as mandated by the system': I'm having trouble parsing 'as'. It is also unclear to me what 'system' means: selinux or PostgreSQL, or both? I suspect it is PostgreSQL, since selinux is still enforcing / 'mandating' it's policy. What about rather than that the switch is controlled by the PostgreSQL server, as in the case of a trusted procedure. +Dynamic domain transitions should be considered carefully, because they +allow users to switch their label, and therefore their privileges, in +at their option, rather than (as in the case of a trusted procedure) +as mandated by the system. -- Yeb Havinga http://www.mgrid.net/ Mastering Medical 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] Command Triggers, v16
On Thursday, March 15, 2012 11:41:21 PM Thom Brown wrote: On 15 March 2012 22:06, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Dimitri Fontaine dimi...@2ndquadrant.fr writes: At this moment in time, CTAS is still outstanding. Is the plan to try to get that in for this release, or as an enhancement in 9.3? The plan is to get CTAS as a utility command in 9.2 then update the command trigger patch to benefit from the new situation. We've been wondering about making its own commit fest entry for that patch, it's now clear in my mind, that needs to happen. https://commitfest.postgresql.org/action/patch_view?id=823 Looks like the ctas-on-command-triggers-01.patch patch needs re-basing. I can do that - but imo the other patch (not based on the command triggers stuff) is the relevant for now as this patch ought to be applied before command triggers. It doesn't seem to make too much sense to rebase it frequently as long as the command triggers patch isn't stable... Any reason you would prefer it being rebased? 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] Command Triggers, v16
* Thom Brown wrote: I don’t understand how functions can return a type of “command trigger”. This certainly works, but I’ve never seen a type consisting of more than one word. Could you explain this for me? This is also postgres= with types (name) as postgres- (select format_type(oid, NULL) from pg_type) postgres- select name from types where name like '% %' postgres- and not name like '%[]'; name - double precision character varying time without time zone timestamp without time zone timestamp with time zone time with time zone bit varying (7 Zeilen) I think these are all specially handled in the parser. -- Christian Ullrich -- 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] Command Triggers, v16
On 16 March 2012 08:13, Andres Freund and...@anarazel.de wrote: On Thursday, March 15, 2012 11:41:21 PM Thom Brown wrote: Looks like the ctas-on-command-triggers-01.patch patch needs re-basing. I can do that - but imo the other patch (not based on the command triggers stuff) is the relevant for now as this patch ought to be applied before command triggers. It doesn't seem to make too much sense to rebase it frequently as long as the command triggers patch isn't stable... Any reason you would prefer it being rebased? Using latest Git master without any additional patches, I can't get it to apply: Hunk #1 FAILED at 16. Hunk #2 succeeded at 22 (offset -1 lines). 1 out of 2 hunks FAILED -- saving rejects to file src/include/commands/tablecmds.h.rej Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Proposal: Create index on foreign table
I have a plan to support 'Create index on foreign table' for 9.3. Here is my plan. The index creation is supported for a flat file such as CSV and a remote table on a RDB e.g., Postgres using CREATE INDEX. (I thought using a new statement, CREATE FOREIGN INDEX, at first, but I think that CREATE INDEX would be sufficient to define an index for the foreign table.) For a flat file, CREATE INDEX constructs an index in the same way as an index for a regular table. On the other hand, for a remote table, CREATE INDEX collects information about the index on the specified column(s) for the specified table that was created on the remote table. An index created is stored in pg_class and pg_index like an index for a regular table. It depends on the wrappers implementation whether it supports the options such as UNIQUE or WHERE predicates, though I think that CONCURRENTLY is not supported in common for the foreign tables. For a flat file, I plan that the user can specify all the options excluding CONCURRENTLY and UNIQUE. On the other hand, for a remote table, I think that the user can specify only the names of the foreign table and its column(s), using which the wrapper collects information about all the related indexes created on the remote table. To do so, I'd like to propose new FDW callback routines: CreateIndex(): This is called maybe from DefineIndex(), and does the similar task to index_create(). For a flat file, this function makes the catalog entries for the index and actually build the index, while for a remote table, it just stores the catalog entries collected from the remote end. DropIndex(): This is called at DROP INDEX, and does the similar task to index_drop(). I'd like to build the index physical data file for a flat file using the index access method of regular tables (ie btree, hash, gin, gist, and spgist) based on the following transformation between the TID and the file offset to some data in the file: block_number = file_offset_to_some_data / BLCKSZ offset_number = file_offset_to_some_data % BLCKSZ I plan to make use of the above index for better query optimization. For a flat file, I'd like to realize index scans, index-only scans, bitmap (like) scans and parametrized scans on the file in the same way as those on a regular table utilizing the currently revised FDW infrastructure. For a remote table, I have to admit that I don't have any clear idea to make use of the index information stored in the system catalogs for better query optimization, but I believe that it's useful for the ORDER BY push down and/or nestloop-with-inner-parametrized-scan join optimization. Thoughts? Best regards, Etsuro Fujita -- 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] Command Triggers, v16
Hi,
On Thursday, March 15, 2012 10:58:49 PM Dimitri Fontaine wrote:
I tricked that in the grammar, the type is called cmdtrigger but I
though it wouldn't be a good choice for the SQL statement.
Hm. I am decidedly unhappy with that grammar hackery... But then maybe I am
squeamish.
+ oid | typname | oid | proname
+--++--+
+ 1790 | refcursor | 46 | textin
+ 3838 | cmdtrigger | 2300 | trigger_in
+(2 rows)
Hm. Wonder if its a good idea to reuse trigger_in. So far we have duplicated
functions for that.
@@ -482,12 +494,21 @@ ListCommandTriggers(CommandContext cmd)
switch (form-ctgtype)
{
case CMD_TRIGGER_FIRED_BEFORE:
- cmd-before = lappend_oid(cmd-before, form-ctgfoid);
+ {
+ if (list_any_triggers)
+ cmd-before_any = lappend_oid(cmd-before_any,
form-ctgfoid); + else
+ cmd-before = lappend_oid(cmd-before, form-ctgfoid);
break;
-
...
+ case CMD_TRIGGER_FIRED_BEFORE:
+ {
+ whenstr = BEFORE;
+
+ foreach(cell, cmd-before_any)
+ {
+ Oid proc = lfirst_oid(cell);
+
+ call_cmdtrigger_procedure(cmd, (RegProcedure)proc,
whenstr); + }
+ foreach(cell, cmd-before)
+ {
+ Oid proc = lfirst_oid(cell);
+
+ call_cmdtrigger_procedure(cmd, (RegProcedure)proc,
whenstr); + }
+ break;
+ }
This will have the effect of calling triggers outside of alphabetic order. I
don't think thats a good idea even if one part is ANY and the other a specific
command.
I don't think there is any reason anymore to separate the two? The only
callsite seems to look like:
632-default:
633:ListCommandTriggers(cmd, true); /* list ANY command
triggers */
634:ListCommandTriggers(cmd, false); /* and triggers for
this
command tag */
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] Command Triggers, v16
On Friday, March 16, 2012 09:30:58 AM Thom Brown wrote: On 16 March 2012 08:13, Andres Freund and...@anarazel.de wrote: On Thursday, March 15, 2012 11:41:21 PM Thom Brown wrote: Looks like the ctas-on-command-triggers-01.patch patch needs re-basing. I can do that - but imo the other patch (not based on the command triggers stuff) is the relevant for now as this patch ought to be applied before command triggers. It doesn't seem to make too much sense to rebase it frequently as long as the command triggers patch isn't stable... Any reason you would prefer it being rebased? Using latest Git master without any additional patches, I can't get it to apply: Hunk #1 FAILED at 16. Hunk #2 succeeded at 22 (offset -1 lines). 1 out of 2 hunks FAILED -- saving rejects to file src/include/commands/tablecmds.h.rej Did you read the paragraph above? 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] Command Triggers, v16
On 15 March 2012 21:58, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Thom Brown thombr...@gmail.com writes: I don’t understand how functions can return a type of “command trigger”. This certainly works, but I’ve never seen a type consisting of more than one word. Could you explain this for me? This is also I tricked that in the grammar, the type is called cmdtrigger but I though it wouldn't be a good choice for the SQL statement. Christian sent me a message mentioning that we've pretty much always had data types consisting of more than one word (e.g. timestamp without time zone). So I completely retract my question as it's obviously nonsense. Thom -- 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] Command Triggers, v16
On 16 March 2012 08:45, Andres Freund and...@anarazel.de wrote: On Friday, March 16, 2012 09:30:58 AM Thom Brown wrote: On 16 March 2012 08:13, Andres Freund and...@anarazel.de wrote: On Thursday, March 15, 2012 11:41:21 PM Thom Brown wrote: Looks like the ctas-on-command-triggers-01.patch patch needs re-basing. I can do that - but imo the other patch (not based on the command triggers stuff) is the relevant for now as this patch ought to be applied before command triggers. It doesn't seem to make too much sense to rebase it frequently as long as the command triggers patch isn't stable... Any reason you would prefer it being rebased? Using latest Git master without any additional patches, I can't get it to apply: Hunk #1 FAILED at 16. Hunk #2 succeeded at 22 (offset -1 lines). 1 out of 2 hunks FAILED -- saving rejects to file src/include/commands/tablecmds.h.rej Did you read the paragraph above? Yes, but I don't think I'm clear on what you mean. Are you saying I should use ctas-01.patch instead of ctas-on-command-triggers-01.patch? If so, that patch results in me not being able to apply Dimitri's command triggers patch. And I thought that patch doesn't actually cause triggers to fire on CTAS? Thom -- 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_terminate_backend for same-role
On Thu, Mar 15, 2012 at 11:01 PM, Daniel Farina dan...@heroku.com wrote: Okay, well, I believe there is a race in handling common administrative signals that *might* possibly matter. In the past, pg_cancel_backend was superuser only, which is a lot like saying only available to people who can be the postgres user and run kill. The cancellation packet is handled via first checking the other backend's BackendKey and then SIGINTing it, leaving only the most narrow possibility for a misdirected SIGINT. Attached is a patch that sketches a removal of the caveat by relying on the BackendId in PGPROC instead of the pid. Basically, the idea is to make it work more like how cancellation keys work, except for internal SQL functions. I think the unsatisfying crux here is the uniqueness of BackendId over the life of one *postmaster* invocation: sinvaladt.c MyBackendId = (stateP - segP-procState[0]) + 1; /* Advertise assigned backend ID in MyProc */ MyProc-backendId = MyBackendId; I'm not sure precisely what to think about how this numbering winds up working on quick inspection. Clearly, if BackendIds can be reused quickly then the pid-race problem comes back in spirit right away. But given the contract of MyBackendId as I understand it (it just has to be unique among all backends at any given time), it could be changed. I don't *think* it's used for its arithmetic relationship to its underlying components anywhere. Another similar solution (not attached) would be to send information about the originating backend through PGPROC and having the check be against those rather than merely a correct and unambiguous MyBackendId. I also see now that cancellation packets does not have this caveat because the postmaster is control of all forking and joining in a serially executed path, so it need not worry about pid racing. Another nice use for this might be to, say, deliver the memory or performance stats of another process while-in-execution, without having to be superuser or and/or gdbing in back to the calling backend. -- fdr From f466fe53e8e64cfa49bf56dbdf5920f9ea4e3562 Mon Sep 17 00:00:00 2001 From: Daniel Farina dan...@heroku.com Date: Thu, 15 Mar 2012 20:46:38 -0700 Subject: [PATCH] Implement race-free sql-originated backend cancellation/termination If promising, this patch requires a few documentation updates in addendum. Since 0495aaad8b337642830a4d4e82f8b8c02b27b1be, pg_cancel_backend can be run on backends that have the same role as the backend pg_cancel_backend is being invoked from. Since that time, a documented caveat exists stating that there was a race whereby a process death-and-recycle could result in an otherwise unrelated backend receiving SIGINT. Now it is desirable for pg_terminate_backend -- which also has the effect of having the backend exit, and closing the socket -- to also be usable by non-superusers. Presuming SIGINT was acceptable to race, it's not clear that it's acceptable for SIGTERM to race in the same way, so this patch seeks to try to do something about that. This patch attempts to close this race condition by targeting query cancellation/termination against the per-backend-startup unique BackendId to unambiguously identify the session rather than a PID. This makes the SQL function act more like how cancellation keys work already (perhaps these paths can be converged somehow). Signed-off-by: Daniel Farina dan...@heroku.com --- src/backend/access/transam/twophase.c |4 + src/backend/storage/ipc/procsignal.c |3 + src/backend/storage/lmgr/proc.c |3 + src/backend/tcop/postgres.c | 55 + src/backend/utils/adt/misc.c | 104 +++- src/include/miscadmin.h |1 + src/include/storage/proc.h| 17 + src/include/storage/procsignal.h |1 + 8 files changed, 146 insertions(+), 42 deletions(-) *** a/src/backend/access/transam/twophase.c --- b/src/backend/access/transam/twophase.c *** *** 62,67 --- 62,68 #include storage/procarray.h #include storage/sinvaladt.h #include storage/smgr.h + #include storage/spin.h #include utils/builtins.h #include utils/memutils.h #include utils/timestamp.h *** *** 326,331 MarkAsPreparing(TransactionId xid, const char *gid, --- 327,335 proc-backendId = InvalidBackendId; proc-databaseId = databaseid; proc-roleId = owner; + SpinLockInit(MyProc-adminMutex); + proc-adminAction = ADMIN_ACTION_NONE; + proc-adminBackendId = InvalidBackendId; proc-lwWaiting = false; proc-lwWaitMode = 0; proc-lwWaitLink = NULL; *** a/src/backend/storage/ipc/procsignal.c --- b/src/backend/storage/ipc/procsignal.c *** *** 258,263 procsignal_sigusr1_handler(SIGNAL_ARGS) --- 258,266 if (CheckProcSignal(PROCSIG_NOTIFY_INTERRUPT)) HandleNotifyInterrupt(); + if (CheckProcSignal(PROCSIG_ADMIN_ACTION_INTERRUPT)) +
Re: [HACKERS] Command Triggers, v16
On Friday, March 16, 2012 09:55:10 AM Thom Brown wrote: On 16 March 2012 08:45, Andres Freund and...@anarazel.de wrote: On Friday, March 16, 2012 09:30:58 AM Thom Brown wrote: On 16 March 2012 08:13, Andres Freund and...@anarazel.de wrote: On Thursday, March 15, 2012 11:41:21 PM Thom Brown wrote: Looks like the ctas-on-command-triggers-01.patch patch needs re-basing. I can do that - but imo the other patch (not based on the command triggers stuff) is the relevant for now as this patch ought to be applied before command triggers. It doesn't seem to make too much sense to rebase it frequently as long as the command triggers patch isn't stable... Any reason you would prefer it being rebased? Using latest Git master without any additional patches, I can't get it to apply: Hunk #1 FAILED at 16. Hunk #2 succeeded at 22 (offset -1 lines). 1 out of 2 hunks FAILED -- saving rejects to file src/include/commands/tablecmds.h.rej Did you read the paragraph above? Yes, but I don't think I'm clear on what you mean. Are you saying I should use ctas-01.patch instead of ctas-on-command-triggers-01.patch? If so, that patch results in me not being able to apply Dimitri's command triggers patch. And I thought that patch doesn't actually cause triggers to fire on CTAS? Well. Why do you want to apply them concurrently? As far as I understand the plan is to get ctas-as-utility merged with master and then let dim rebase the ddl trigger patch. The ctas-as-utility stuff imo is worthy of being applied independently of DDL triggers. The current duplication in the code lead to multiple bugs already. 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] Command Triggers, v16
Andres Freund and...@anarazel.de writes: On Thursday, March 15, 2012 10:58:49 PM Dimitri Fontaine wrote: I tricked that in the grammar, the type is called cmdtrigger but I though it wouldn't be a good choice for the SQL statement. Hm. I am decidedly unhappy with that grammar hackery... But then maybe I am squeamish. It's easy to remove that hack and get back to having the user visible type be cmdtrigger, but as this type only serves as a marker for PL compiling function (validate handler) I though having a user friendly name was important here. + oid | typname | oid | proname +--++--+ + 1790 | refcursor | 46 | textin + 3838 | cmdtrigger | 2300 | trigger_in +(2 rows) Hm. Wonder if its a good idea to reuse trigger_in. So far we have duplicated functions for that. Again, if we think the use case warrants duplicating code rather than playing with the type definition, I will do that. This will have the effect of calling triggers outside of alphabetic order. I don't think thats a good idea even if one part is ANY and the other a specific command. I don't think there is any reason anymore to separate the two? The only callsite seems to look like: The idea is to have a predictable ordering of command triggers. The code changed in the patch v16 (you pasted code from git in between v15 and v16, I cleaned it up) and is now easier to read: case CMD_TRIGGER_FIRED_BEFORE: whenstr = BEFORE; procs[0] = cmd-before_any; procs[1] = cmd-before; break; case CMD_TRIGGER_FIRED_AFTER: whenstr = AFTER; procs[0] = cmd-after; procs[1] = cmd-after_any; break; So it's BEFORE ANY then BEFORE command then AFTER command then AFTER ANY. That's an arbitrary I made and we can easily reconsider. Triggers are called in alphabetical order in each “slot” here. In my mind it makes sense to have ANY triggers around the specific triggers, but it's hard to explain why that feels better. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- 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: Create index on foreign table
On 16.03.2012 10:44, Etsuro Fujita wrote: I have a plan to support 'Create index on foreign table' for 9.3. Here is my plan. The index creation is supported for a flat file such as CSV and a remote table on a RDB e.g., Postgres using CREATE INDEX. (I thought using a new statement, CREATE FOREIGN INDEX, at first, but I think that CREATE INDEX would be sufficient to define an index for the foreign table.) For a flat file, CREATE INDEX constructs an index in the same way as an index for a regular table. I think this belongs completely in the remote data source. If you want to index flat files, create an extra file for it or something, and enhance the wrapper so that it can take advantage of it. Keeping the index inside the database while the data is somewhere else creates a whole new class of problems. For starters, how would you keep the index up-to-date when the flat file is modified? If you want to take advantage of PostgreSQL's indexing, you'll just have to just load the data into a regular table. On the other hand, for a remote table, CREATE INDEX collects information about the index on the specified column(s) for the specified table that was created on the remote table. I don't see the point of this either. The planner asks the FDW for cost estimates, and if the FDW knows about indexes in the remote server, it can certainly adjust the estimates accordingly. But that's all internal to the FDW. It might want delegate the whole cost estimation to the remote server by running EXPLAIN there, or it could use its knowledge of indexes that exist there, but I don't see why the rest of the system would need to know what indexes there are in the remote system. If the FDW needs that information, it can query the remote server for it on first access, and cache the information for the lifetime of the session. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.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] initdb and fsync
On Thursday, March 15, 2012 07:38:38 AM Jeff Davis wrote: On Wed, 2012-03-14 at 10:26 +0100, Andres Freund wrote: On Wednesday, March 14, 2012 05:23:03 AM Jeff Davis wrote: On Tue, 2012-03-13 at 09:42 +0100, Andres Freund wrote: for recursively everything in dir: posix_fadvise(fd, POSIX_FADV_DONTNEED); for recursively everything in dir: fsync(fd); Wow, that made a huge difference! no sync: ~ 1.0s sync: ~10.0s fadvise+sync: ~ 1.3s I take that back. There was something wrong with my test -- fadvise helps, but it only takes it from ~10s to ~6.5s. Not quite as good as I hoped. Thats surprising. I wouldn't expect such a big difference between fadvise + sync_file_range. Rather strange. Well, while the positive effect of this are rather large it also has the bad effect of pushing the whole new database out of the cache. Which is not so nice if you want to run tests on it seconds later. I was unable to see a regression when it comes to starting it up after the fadvise+fsync. My test just started the server, created a table, then stopped the server. It was actually a hair faster with the directory that had been fadvise'd and then fsync'd, but I assume that was noise. Regardless, this doesn't look like an issue. Hm. Ok. How are the results with sync_file_range(fd, 0, 0, SYNC_FILE_RANGE_WRITE)? That is much faster than using fadvise. It goes down to ~2s. Unfortunately, that's non-portable. Any other ideas? 6.5s a little on the annoying side (and causes some disconcerting sounds to come from my disk), especially when we _know_ it can be done in 2s. Its not like posix_fadvise is actually portable. So I personally don't see a problem with that, but... Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Why does exprCollation reject List node?
Hi all, During writing pgsql_fdw codes, I noticed that exprCollation rejects non-Expr nodes with error unrecognized node type: %d. Is this intentional behavior, or can it return InvalidOid for unrecognized nodes like exprInputCollation? Background information: I use exprCollation with expression_walker in pgsql_fdw to know whether an expression in baserestrictinfo-clause list uses any collation, to determine the clause can be pushed down safely. I want to allow pushing ScalarArrayOpExpr down, but its argument is represented as List, so a query contains ScalarArrayOpExpr ends with error when the traversing expression tree reaches arguments of ScalarArrayOpExpr. I've spent only few hours for research though, but the interface of exprCollation seems little odd. It accepts Node*, but its switch statement assumes that given object should be something derived from Expr, and it rejects other objects with elog(ERROR). Anyone know the reason? Regards, -- Shigeru Hanada -- 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] Command Triggers, v16
Thom Brown thombr...@gmail.com writes:
Note: incremental patch attached for the following section...
Applied, thanks!
I don’t know if this was a problem before that I didn’t spot
(probably), but triggers for both ANY COMMAND and ALTER FOREIGN TABLE
show a command tag of ALTER TABLE for ALTER FOREIGN TABLE statements
where the column is renamed:
I don’t think this is the fault of the trigger code because it
actually says ALTER TABLE at the bottom, suggesting it’s something
already present. This isn’t the case when adding or dropping columns.
Any comments?
We're building command trigger on top of the existing code. From the
grammar we can see that an alter table and an alter foreign table are
processed as the same command.
AlterForeignTableStmt:
ALTER FOREIGN TABLE relation_expr alter_table_cmds
{
AlterTableStmt *n = makeNode(AlterTableStmt);
So while I think we could want to revisit that choice down the road, I
don't think that's for the command triggers patch to do it.
Altering the properties of a function (such as cost, security definer,
whether it’s stable etc) doesn’t report the function’s OID:
That's now fixed. I've checked that all the other places where we're
saying objectId = InvalidOid are related to before create or after drop
commands.
I get a garbage objectname for AFTER ANY COMMAND triggers on ALTER
TEXT SEARCH DICTIONARY when changing its options. It doesn’t show it
in the below example because I can’t get it displaying in plain text,
but where the objectname is blank is where I’m seeing unicode a square
containing “0074” 63 times in a row:
I couldn't reproduce, but I've spotted the problem in the source, and
fixed it. I could find a couple other places that should have been using
pstrdup(NameStr(…)) too, and fixed them. I added a test.
Specific command triggers on ALTER VIEW don’t work at all:
Can't reproduce, and that's already part of the regression tests.
Command triggers that fire for CREATE RULE show a schema, but DROP
RULE doesn’t. Which is it?:
Oh, both RULE and TRIGGER are not qualified, and I was filling the
schemaname with the schema of the relation they refer to in the CREATE
command, and had DROP correctly handling the TRIGGER case.
That's now fixed, schemaname is NULL in all cases here.
You can read the changes here:
https://github.com/dimitri/postgres/compare/5e8e37922d...144d870162
And I've been attaching an incremental patch too. Next patch revision is
expected later today with support for plpython, plperl and pltcl.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index f5f2079..5f84751 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3933,20 +3933,19 @@ SELECT * FROM sales_summary_bytime;
titleTriggers on commands/title
para
-applicationPL/pgSQL/application can be used to define trigger
-procedures. A trigger procedure is created with the
+applicationPL/pgSQL/application can be used to define command
+trigger procedures. A command trigger procedure is created with the
commandCREATE FUNCTION/ command, declaring it as a function with
no arguments and a return type of typecommand trigger/type.
/para
para
When a applicationPL/pgSQL/application function is called as a
- trigger, several special variables are created automatically in the
- top-level block. They are:
+ command trigger, several special variables are created automatically
+ in the top-level block. They are:
variablelist
varlistentry
-varlistentry
termvarnameTG_TAG/varname/term
listitem
para
@@ -4002,7 +4001,7 @@ SELECT * FROM sales_summary_bytime;
/para
para
- The command trigger function return's value is not used.
+ The command trigger function's return value is not used.
/para
para
@@ -4014,23 +4013,20 @@ SELECT * FROM sales_summary_bytime;
titleA applicationPL/pgSQL/application Command Trigger Procedure/title
para
- This example trigger just raise a literalNOTICE/literal message
+ This example trigger simply raises a literalNOTICE/literal message
each time a supported command is executed.
/para
programlisting
-create or replace function snitch()
- returns command trigger
- language plpgsql
-as $$
-begin
- raise notice 'snitch: % % %.% [%]',
+CREATE OR REPLACE FUNCTION snitch() RETURNS command trigger AS $$
+BEGIN
+RAISE NOTICE 'snitch: % % %.% [%]',
tg_when, tg_tag, tg_schemaname, tg_objectname, tg_objectid;
-end;
-$$;
+END;
+$$ LANGUAGE plpgsql;
-create command trigger snitch_before before any command execute procedure any_snitch();
-create command trigger snitch_after after any command execute procedure any_snitch();
+CREATE COMMAND TRIGGER snitch_before BEFORE ANY COMMAND EXECUTE PROCEDURE snitch();
+CREATE COMMAND
Re: [HACKERS] Command Triggers, v16
On 16 March 2012 11:42, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Thom Brown thombr...@gmail.com writes: Specific command triggers on ALTER VIEW don’t work at all: Can't reproduce, and that's already part of the regression tests. This was a problem my side (a mistake I made previously) as I hadn't added this particular one into my list of created command triggers. I had then seen the triggers fire, but forgot to go back and remove my statement from the draft email. Apologies. And I've been attaching an incremental patch too. Next patch revision is expected later today with support for plpython, plperl and pltcl. Okay, I shalln't do any more testing until the next patch. I should probably have worked on automating my tests more, but never got round to it. Thom -- 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] Command Triggers, v16
Thom Brown thombr...@gmail.com writes: Okay, I shalln't do any more testing until the next patch. I should probably have worked on automating my tests more, but never got round to it. make installcheck :) That said, your test allow to spot OID problems that we can't add in the regression tests (OID being too volatile would break them), and I have to look at how to add regression tests for the other pls support… Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- 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] Command Triggers, v16
On 16 March 2012 12:07, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Thom Brown thombr...@gmail.com writes: Okay, I shalln't do any more testing until the next patch. I should probably have worked on automating my tests more, but never got round to it. make installcheck :) That said, your test allow to spot OID problems that we can't add in the regression tests (OID being too volatile would break them), and I have to look at how to add regression tests for the other pls support… Yes, that's why I don't use the regression tests. :) Thom -- 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: Create index on foreign table
(2012/03/16 18:58), Heikki Linnakangas wrote: On 16.03.2012 10:44, Etsuro Fujita wrote: I have a plan to support 'Create index on foreign table' for 9.3. Here is my plan. The index creation is supported for a flat file such as CSV and a remote table on a RDB e.g., Postgres using CREATE INDEX. (I thought using a new statement, CREATE FOREIGN INDEX, at first, but I think that CREATE INDEX would be sufficient to define an index for the foreign table.) For a flat file, CREATE INDEX constructs an index in the same way as an index for a regular table. I think this belongs completely in the remote data source. If you want to index flat files, create an extra file for it or something, and enhance the wrapper so that it can take advantage of it. Keeping the index inside the database while the data is somewhere else creates a whole new class of problems. For starters, how would you keep the index up-to-date when the flat file is modified? Index update is outside the scope at least until foreign table update is supported. It is required for the user to do DROP INDEX and then do CREATE INDEX again when the file has been modified. I plan to implement the GetForeignPaths callback routine of file_fdw to just give up creating index paths if the file's checksum or timestamp or something has changed. I think the index of the file is something similar to the stats of it in a certain sense. On the other hand, for a remote table, CREATE INDEX collects information about the index on the specified column(s) for the specified table that was created on the remote table. I don't see the point of this either. The planner asks the FDW for cost estimates, and if the FDW knows about indexes in the remote server, it can certainly adjust the estimates accordingly. But that's all internal to the FDW. It might want delegate the whole cost estimation to the remote server by running EXPLAIN there, or it could use its knowledge of indexes that exist there, but I don't see why the rest of the system would need to know what indexes there are in the remote system. In the case of joining A on the local system and B on the remote system, for example, it is required for the local system to know what indexes there are on B in the remote system in order to consider index paths parametrized by A for nestloop-with-inner-parametrized-scan paths. If the FDW needs that information, it can query the remote server for it on first access, and cache the information for the lifetime of the session. I think that is one of the choices. However, it seems convenient to me to utilize the existing framework for index information because this approach has the possibility for the FDWs to share e.g., the path creation processing with Postgres core to some extent to create index paths, of course which requires to refactor existing query optimization code. Best regards, Etsuro Fujita -- 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] Syntax error and reserved keywords
Peter Eisentraut pete...@gmx.net writes: Is there a reason for us not to add an HINT: user is a reserved keyword or something like that, other than nobody having been interested in doing the work? If that were easily possible, we could just recognize 'user' as an identifier in this context and avoid the issue altogether. But it's not. Thanks, I guess I see the logic here. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- 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] Why does exprCollation reject List node?
Shigeru HANADA shigeru.han...@gmail.com writes: During writing pgsql_fdw codes, I noticed that exprCollation rejects non-Expr nodes with error unrecognized node type: %d. Is this intentional behavior, or can it return InvalidOid for unrecognized nodes like exprInputCollation? Doesn't seem to me that asking for the collation of a list is very sensible, so I don't see a problem with that. Background information: I use exprCollation with expression_walker in pgsql_fdw to know whether an expression in baserestrictinfo-clause list uses any collation, to determine the clause can be pushed down safely. Returning InvalidOid in such a case would be the *wrong answer*, because it would presumably lead the code to conclude that nothing within the list has a collation, which ain't necessarily so. 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] Syntax error and reserved keywords
On 16.03.2012 14:50, Dimitri Fontaine wrote: Peter Eisentrautpete...@gmx.net writes: Is there a reason for us not to add an HINT: user is a reserved keyword or something like that, other than nobody having been interested in doing the work? If that were easily possible, we could just recognize 'user' as an identifier in this context and avoid the issue altogether. But it's not. Thanks, I guess I see the logic here. Accepting the keyword in such a context seems much harder to me than providing a hint. To accept the keyword, you'd need a lot of changes to the grammar, but for the hint, you just need some extra code in yyerror(). Mind you, if it's a hint, it doesn't need to be 100% accurate, so I think you could just always give the hint if you get a grammar error at a token that's a reserved keyword. Even if it was easy to accept the keywords when there's no ambiguity, I don't think we would want that. Currently, we can extend the syntax using existing keywords, knowing that we don't break existing applications, but that would no longer be true if reserved keywords were sometimes accepted as identifiers. For example, imagine that you had this in your application: CREATE TABLE foo (bar order); Order is a reserved keyword so that doesn't work currently, but we could accept it as an identifier in this context. But if we then decided to extend the syntax, for example to allow ORDER as a synonym for serial in CREATE TABLE clauses, that would stop working. We currently avoid introducing new reserved keywords, because that can break existing applications, but if we started to accept existing keywords as identifiers in some contexts, we would have to be more careful with even extending the use of existing keywords. However, I like the idea of a hint, so +1 for Dimitri's original suggestion. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.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] foreign key locks, 2nd attempt
Excerpts from Bruce Momjian's message of vie mar 16 00:04:06 -0300 2012: On Tue, Mar 13, 2012 at 02:35:02PM -0300, Alvaro Herrera wrote: Excerpts from Bruce Momjian's message of mar mar 13 14:00:52 -0300 2012: On Tue, Mar 06, 2012 at 04:39:32PM -0300, Alvaro Herrera wrote: When there is a single locker in a tuple, we can just store the locking info in the tuple itself. We do this by storing the locker's Xid in XMAX, and setting hint bits specifying the locking strength. There is one exception here: since hint bit space is limited, we do not provide a separate hint bit for SELECT FOR SHARE, so we have to use the extended info in a MultiXact in that case. (The other cases, SELECT FOR UPDATE and SELECT FOR KEY SHARE, are presumably more commonly used due to being the standards-mandated locking mechanism, or heavily used by the RI code, so we want to provide fast paths for those.) Are those tuple bits actually hint bits? They seem quite a bit more powerful than a hint. I'm not sure what's your point. We've had a hint bit for SELECT FOR UPDATE for ages. Even 8.2 had HEAP_XMAX_EXCL_LOCK and HEAP_XMAX_SHARED_LOCK. Maybe they are misnamed and aren't really hints, but it's not the job of this patch to fix that problem. Now I am confused. Where do you see the word hint used by HEAP_XMAX_EXCL_LOCK and HEAP_XMAX_SHARED_LOCK. These are tuple infomask bits, not hints, meaning they are not optional or there just for performance. Okay, I think this is just a case of confusing terminology. I have always assumed (because I have not seen any evidence to the contrary) that anything in t_infomask and t_infomask2 is a hint bit -- regardless of it being actually a hint or something with a stronger significance. HEAP_XMAX_EXCL_LOCK and HEAP_XMAX_SHARED_LOCK are certainly not optional in the sense that if they are missing, the meaning of the Xmax field is completely different. So in all correctness they are not hints, though we call them that. Now, if we want to differentiate infomask bits that are just hints from those that are something else, we can do that, but I'm not sure it's useful -- AFAICS only XMAX_COMMITTED and XMIN_COMMITTED are proper hints. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] foreign key locks, 2nd attempt
Excerpts from Alvaro Herrera's message of vie mar 16 10:36:11 -0300 2012: Now I am confused. Where do you see the word hint used by HEAP_XMAX_EXCL_LOCK and HEAP_XMAX_SHARED_LOCK. These are tuple infomask bits, not hints, meaning they are not optional or there just for performance. Okay, I think this is just a case of confusing terminology. I have always assumed (because I have not seen any evidence to the contrary) that anything in t_infomask and t_infomask2 is a hint bit -- regardless of it being actually a hint or something with a stronger significance. Maybe this is just my mistake. I see in http://wiki.postgresql.org/wiki/Hint_Bits that we only call the COMMITTED/INVALID infomask bits hints. I think it's easy enough to correct the README to call them infomask bits rather than hints .. I'll go do that. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Command Triggers, v16
Andres Freund and...@anarazel.de writes: On Thursday, March 15, 2012 10:58:49 PM Dimitri Fontaine wrote: I tricked that in the grammar, the type is called cmdtrigger but I though it wouldn't be a good choice for the SQL statement. Hm. I am decidedly unhappy with that grammar hackery... But then maybe I am squeamish. Multi-word type names are a serious pain in the ass; they require hackery in a lot of places. We support the ones that the SQL spec requires us to, but I will object in the strongest terms to inventing any that are not required by spec. I object in even stronger terms to the incredibly klugy way you did it here. If you think cmdtrigger isn't a good name maybe you should have picked a different one to start with. While I'm looking at the grammar ... it also seems like a serious PITA from a maintenance standpoint that we're now going to have to adjust the CREATE COMMAND TRIGGER productions every time somebody thinks of a new SQL command. Maybe we should drop this whole idea of specifying which commands a trigger acts on at the SQL level, and just have one-size-fits-all command triggers. Or perhaps have the selection be on the basis of strings that are matched to command tags, instead of grammar constructs. 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] Proposal: Create index on foreign table
2012/3/16 Etsuro Fujita fujita.ets...@lab.ntt.co.jp: I have a plan to support 'Create index on foreign table' for 9.3. Here is my plan. Very interesting idea, but... The index creation is supported for a flat file such as CSV and a remote table on a RDB e.g., Postgres using CREATE INDEX. Why do you limit the target type to those two? How about web services and non-relational databases? Some web services would provide id-to-content mapping, and KVSs are obviously accessible by key. IMHO CREATE INDEX for foreign tables should have general design, not specific to some kind of FDWs. I'd like to build the index physical data file for a flat file using the index access method of regular tables (ie btree, hash, gin, gist, and spgist) based on the following transformation between the TID and the file offset to some data in the file: block_number = file_offset_to_some_data / BLCKSZ offset_number = file_offset_to_some_data % BLCKSZ Indeed these information would help searching data stored in local files. But, again, it seems too specific to file-based FDWs. I'd suggest separating basic general design and implementation by FDWs. The design you shown here seems indexed-file_fdw to me... Regards, -- Shigeru HANADA -- 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] Command Triggers, v16
On Friday, March 16, 2012 02:50:55 PM Tom Lane wrote: While I'm looking at the grammar ... it also seems like a serious PITA from a maintenance standpoint that we're now going to have to adjust the CREATE COMMAND TRIGGER productions every time somebody thinks of a new SQL command. I don't find that argument really convincing. The current state of the patch will often require attention to command triggers anyway... 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] Command Triggers, v16
Andres Freund and...@anarazel.de writes: On Friday, March 16, 2012 02:50:55 PM Tom Lane wrote: While I'm looking at the grammar ... it also seems like a serious PITA from a maintenance standpoint that we're now going to have to adjust the CREATE COMMAND TRIGGER productions every time somebody thinks of a new SQL command. I don't find that argument really convincing. Well, how about just plain parser size bloat? Did anyone look at how much bigger gram.o becomes with this? Bigger parser - slower, for everybody, whether they care about this feature or not. 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] foreign key locks, 2nd attempt
On Thu, Mar 15, 2012 at 11:09 PM, Bruce Momjian br...@momjian.us wrote: On Tue, Mar 13, 2012 at 01:46:24PM -0400, Robert Haas wrote: On Mon, Mar 12, 2012 at 3:28 PM, Simon Riggs si...@2ndquadrant.com wrote: I agree with you that some worst case performance tests should be done. Could you please say what you think the worst cases would be, so those can be tested? That would avoid wasting time or getting anything backwards. I've thought about this some and here's what I've come up with so far: I question whether we are in a position to do the testing necessary to commit this for 9.2. Is anyone even working on testing 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] Syntax error and reserved keywords
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: Accepting the keyword in such a context seems much harder to me than providing a hint. To accept the keyword, you'd need a lot of changes to the grammar, but for the hint, you just need some extra code in yyerror(). Mind you, if it's a hint, it doesn't need to be 100% accurate, so I think you could just always give the hint if you get a grammar error at a token that's a reserved keyword. Unfortunately, while a useful hint doesn't have to be 100% right, it does have to be a great deal more than 0% right. And what you're suggesting here would be nearly all noise. For example, if I write SELECT ORDER BY x; it is not going to be helpful to be told that ORDER is a reserved word. It will soon become annoying for that hint to pop up in many contexts where it's completely inappropriate. If you could restrict it to only happen in contexts where the *only* expected token is an identifier, it might be of some use, but I'm doubtful that yyerror() has that much info. There is some stuff in the Bison manual about writing error productions, which I've never paid much attention to because it only seemed to be useful for resychronizing between statements. But maybe there's something there for this purpose. Even if it was easy to accept the keywords when there's no ambiguity, I don't think we would want that. Currently, we can extend the syntax using existing keywords, knowing that we don't break existing applications, but that would no longer be true if reserved keywords were sometimes accepted as identifiers. Good point. 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] Command Triggers, v16
Tom Lane t...@sss.pgh.pa.us writes: Multi-word type names are a serious pain in the ass; they require hackery in a lot of places. We support the ones that the SQL spec requires us to, but I will object in the strongest terms to inventing any that are not required by spec. I object in even stronger terms to the incredibly klugy way you did it here. Ok, it's so klugy that removing the support from the parser is going to be easy. If you think cmdtrigger isn't a good name maybe you should have picked a different one to start with. Well, I think it's a good internal name. I'm not too sure about exposing it, the only reason why it's a good name is because it's a single not too long word, after all. Not very “SQLish”. I'm putting cmdtrigger as the user visible name in the next version of the patch, if you come up with something potentially more user friendly feel free to suggest. While I'm looking at the grammar ... it also seems like a serious PITA from a maintenance standpoint that we're now going to have to adjust the CREATE COMMAND TRIGGER productions every time somebody thinks of a new SQL command. Maybe we should drop this whole idea of specifying which commands a trigger acts on at the SQL level, and just have one-size-fits-all command triggers. Or perhaps have the selection be on the basis of strings that are matched to command tags, instead of grammar constructs. The only advantage of doing it this way is giving the user an early error when he's trying to attach to a non-supported command. I wouldn't want to remove that list only to find myself adding a list of non supported commands so that we can still refuse creating the useless command trigger. And as Andres said, adding command trigger support to a new command isn't exactly transparent (it's still mostly mechanical though), so that does not seems so big a pain to me. Of course I have been having my head in there for a long time now… Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- 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] EquivalenceClasses and subqueries and PlaceHolderVars, oh my
I wrote:
So I now propose reverting the earlier two patches (but not their
regression test cases of course) and instead hacking MergeAppend plan
building as per (2).
Attached is a draft patch for that. There are several things going on
here:
* Revert the preceding two patches (except for their regression test
cases).
* Install defenses to ensure that child EC members are ignored except in
very specific contexts, and improve documentation around that. The most
significant change is that get_eclass_for_sort_expr now takes a Relids
argument, and won't consider child members unless they match the Relids.
It turns out that the places that were trying to match indexes to ECs
already acted that way; which I think I had done just as a speed
optimization, but it turns out to be important for correctness too.
* Rewrite prepare_sort_from_pathkeys() to allow required sort column
numbers to be passed in, thus fixing Teodor's original problem more
directly. As part of that, I removed createplan.c's add_sort_column()
code, which was meant as a last-ditch check that we don't build sorts
with useless duplicate sort columns. As far as I can tell, that's dead
code and has been for a long time, because we already eliminate
duplicate pathkeys or SortGroupClause list entries far upstream of this.
Even if there are some corner cases where it does something useful,
that's rare enough to not really justify expending the cycles. I had to
remove it because if it did fire and remove a sort column, the outer
loop in prepare_sort_from_pathkeys() wouldn't be in sync with the input
column-number array.
* While testing this I noticed that planagg.c failed to optimize min/max
aggregates on appendrels that are made from UNION ALL subqueries rather
than inherited relations. So this patch also tweaks planagg.c to handle
such cases.
Viewing the problem this way, the only known symptom is the originally
reported MergeAppend child's targetlist doesn't match MergeAppend,
which of course is not an issue before 9.1, so I'm not going to try
to back-patch further than 9.1. It is possible that we need some of the
child EC member defenses further back, but I'll await some evidence of
user-visible bugs first.
regards, tom lane
diff --git a/src/backend/optimizer/README b/src/backend/optimizer/README
index ee450fb02e4e9e8942ff5a0d6b062c9ae5d18cba..9ab1fdf8154b964be91eb54ff2a3f38e9705af74 100644
*** a/src/backend/optimizer/README
--- b/src/backend/optimizer/README
*** it's possible that it belongs to more th
*** 496,501
--- 496,509
families to ensure that we can make use of an index belonging to any one of
the families for mergejoin purposes.)
+ An EquivalenceClass can contain em_is_child members, which are copies
+ of members that contain appendrel parent relation Vars, transposed to
+ contain the equivalent child-relation variables or expressions. These
+ members are *not* full-fledged members of the EquivalenceClass and do not
+ affect the class's overall properties at all. They are kept only to
+ simplify matching of child-relation expressions to EquivalenceClasses.
+ Most operations on EquivalenceClasses should ignore child members.
+
PathKeys
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index b653d6cb35ca338c183bb944757081e46ca50c50..c115c2148db109476dd607f99a178e25a5f76a24 100644
*** a/src/backend/optimizer/path/equivclass.c
--- b/src/backend/optimizer/path/equivclass.c
*** add_eq_member(EquivalenceClass *ec, Expr
*** 491,496
--- 491,505
* sortref is the SortGroupRef of the originating SortGroupClause, if any,
* or zero if not. (It should never be zero if the expression is volatile!)
*
+ * If rel is not NULL, it identifies a specific relation we're considering
+ * a path for, and indicates that child EC members for that relation can be
+ * considered. Otherwise child members are ignored. (Note: since child EC
+ * members aren't guaranteed unique, a non-NULL value means that there could
+ * be more than one EC that matches the expression; if so it's order-dependent
+ * which one you get. This is annoying but it only happens in corner cases,
+ * so for now we live with just reporting the first match. See also
+ * generate_implied_equalities_for_indexcol and match_pathkeys_to_index.)
+ *
* If create_it is TRUE, we'll build a new EquivalenceClass when there is no
* match. If create_it is FALSE, we just return NULL when no match.
*
*** get_eclass_for_sort_expr(PlannerInfo *ro
*** 511,516
--- 520,526
Oid opcintype,
Oid collation,
Index sortref,
+ Relids rel,
bool create_it)
{
EquivalenceClass *newec;
*** get_eclass_for_sort_expr(PlannerInfo *ro
*** 549,554
--- 559,571
EquivalenceMember *cur_em = (EquivalenceMember *) lfirst(lc2);
/*
+ * Ignore child members
Re: [HACKERS] Command Triggers, v16
On Fri, Mar 16, 2012 at 7:42 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: I don’t know if this was a problem before that I didn’t spot (probably), but triggers for both ANY COMMAND and ALTER FOREIGN TABLE show a command tag of ALTER TABLE for ALTER FOREIGN TABLE statements where the column is renamed: I don’t think this is the fault of the trigger code because it actually says ALTER TABLE at the bottom, suggesting it’s something already present. This isn’t the case when adding or dropping columns. Any comments? We're building command trigger on top of the existing code. From the grammar we can see that an alter table and an alter foreign table are processed as the same command. This seems pretty dicey. I understand your position that it can't be the job of the command triggers patch to fix every infelicity of the backend, but on the flip side, code reuse is a good thing. We want to increase, not decrease, the number of places where the same code can be used to implement multiple commands that do related things; and there has to be some way to do that without breaking command triggers. Moreover, we really don't want the details of how things are handled internally to be user-visible, because sometimes we refactor things to improve the quality of our code, and I don't want to get bug reports when we do that... -- 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] Command Triggers, v16
Robert Haas robertmh...@gmail.com writes: there has to be some way to do that without breaking command triggers. Sure, special case the switch branch in utility.c so as to return a different command tag for ALTER TABLE and ALTER FOREIGN TABLE. For precedents, see AlterObjectTypeCommandTag(ObjectType objtype) and case T_DropStmt: switch (((DropStmt *) parsetree)-removeType) case T_DefineStmt: switch (((DefineStmt *) parsetree)-kind) So, do we want to do the same thing for ALTER FOREIGN TABLE, and should I do that in the command triggers patch? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- 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] initdb and fsync
On Fri, Mar 16, 2012 at 6:25 AM, Andres Freund and...@anarazel.de wrote: How are the results with sync_file_range(fd, 0, 0, SYNC_FILE_RANGE_WRITE)? That is much faster than using fadvise. It goes down to ~2s. Unfortunately, that's non-portable. Any other ideas? 6.5s a little on the annoying side (and causes some disconcerting sounds to come from my disk), especially when we _know_ it can be done in 2s. Its not like posix_fadvise is actually portable. So I personally don't see a problem with that, but... Well, sync_file_range only works on Linux, and will probably never work anywhere else. posix_fadvise() at least has a chance of being supported on other platforms, being a standard and all that. Though I see that my Mac has neither. :-( -- 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] initdb and fsync
On Friday, March 16, 2012 04:47:06 PM Robert Haas wrote: On Fri, Mar 16, 2012 at 6:25 AM, Andres Freund and...@anarazel.de wrote: How are the results with sync_file_range(fd, 0, 0, SYNC_FILE_RANGE_WRITE)? That is much faster than using fadvise. It goes down to ~2s. Unfortunately, that's non-portable. Any other ideas? 6.5s a little on the annoying side (and causes some disconcerting sounds to come from my disk), especially when we _know_ it can be done in 2s. Its not like posix_fadvise is actually portable. So I personally don't see a problem with that, but... Well, sync_file_range only works on Linux, and will probably never work anywhere else. posix_fadvise() at least has a chance of being supported on other platforms, being a standard and all that. Though I see that my Mac has neither. :-( I would suggest adding a wrapper function like: pg_hint_writeback_flush(fd, off, len); which then is something like #if HAVE_SYNC_FILE_RANGE sync_file_range(fd, off, len, SYNC_FILE_RANGE_WRITE); #elseif HAVE_POSIX_FADVISE posix_fadvise(fd, off, len, POSIX_FADV_DONTNEED); #else #endif To my knowledge posix_fadvise currently is only supported on linux btw... 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] Command Triggers, v16
Dimitri Fontaine dimi...@2ndquadrant.fr writes: Tom Lane t...@sss.pgh.pa.us writes: If you think cmdtrigger isn't a good name maybe you should have picked a different one to start with. Well, I think it's a good internal name. I'm not too sure about exposing it, the only reason why it's a good name is because it's a single not too long word, after all. Not very âSQLishâ. I'm putting cmdtrigger as the user visible name in the next version of the patch, if you come up with something potentially more user friendly feel free to suggest. How about commandtrigger or command_trigger? Typing a few more characters in this context doesn't seem like a deal-breaker to me. 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] Proposal: Create index on foreign table
On Fri, Mar 16, 2012 at 11:58:29AM +0200, Heikki Linnakangas wrote: On 16.03.2012 10:44, Etsuro Fujita wrote: I have a plan to support 'Create index on foreign table' for 9.3. Here is my plan. The index creation is supported for a flat file such as CSV and a As others, I don't see a reason to restrict this to some particular type of FDW. remote table on a RDB e.g., Postgres using CREATE INDEX. (I thought using a new statement, CREATE FOREIGN INDEX, at first, but I think that CREATE INDEX would be sufficient to define an index for the foreign table.) For a flat file, CREATE INDEX constructs an index in the same way as an index for a regular table. I think this belongs completely in the remote data source. I think this needs to be decided on a case-by-case basis instead. If you want to index flat files, create an extra file for it or something, and enhance the wrapper so that it can take advantage of it. Keeping the index inside the database while the data is somewhere else creates a whole new class of problems. How? These aren't super different from those for, say, unlogged tables. For starters, how would you keep the index up-to-date when the flat file is modified? One way is to poll the remote source for evidence of such changes during auto_vacuum or with similar daemon processes. Another is by waiting for a signal from an external source such as a NOTIFY. Which is more appropriate will again depend on circumstances. If you want to take advantage of PostgreSQL's indexing, you'll just have to just load the data into a regular table. I disagree. Indexing in general allows you to store only log-N index rows for each N rows in an external table, which could be a very big win. Deciding in advance for everyone that this is not worthwhile is not in our purview. On the other hand, for a remote table, CREATE INDEX collects information about the index on the specified column(s) for the specified table that was created on the remote table. I don't see the point of this either. The planner asks the FDW for cost estimates, and if the FDW knows about indexes in the remote server, it can certainly adjust the estimates accordingly. But that's all internal to the FDW. It might want delegate the whole cost estimation to the remote server by running EXPLAIN there, or it could use its knowledge of indexes that exist there, but I don't see why the rest of the system would need to know what indexes there are in the remote system. Good point, for the remote index case, which I contend is not every one :) If the FDW needs that information, it can query the remote server for it on first access, and cache the information for the lifetime of the session. Of course, a mere few GB of information queried each time couldn't possibly cause intolerable overheads... Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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: autocomplete for functions
On tor, 2012-03-15 at 16:36 -0300, Alvaro Herrera wrote: Excerpts from Peter Eisentraut's message of jue mar 15 16:25:53 -0300 2012: On sön, 2012-02-19 at 20:10 +0100, Pavel Stehule wrote: I found so this extremely simple patch should be useful. It helps for pattern SELECT fx(); Isn't that just a subset of what I had proposed? http://archives.postgresql.org/message-id/1328820579.11241.4.ca...@vanquo.pezone.net So do you intend to commit your patch? Well, there was quite a bit of discussion about it, but it appears that most concerns were addressed at the end. So yes, I guess, unless someone wants further discussion. -- 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] initdb and fsync
On Fri, 2012-03-16 at 11:25 +0100, Andres Freund wrote: I take that back. There was something wrong with my test -- fadvise helps, but it only takes it from ~10s to ~6.5s. Not quite as good as I hoped. Thats surprising. I wouldn't expect such a big difference between fadvise + sync_file_range. Rather strange. I discussed this with my colleague who knows linux internals, and he pointed me directly at the problem. fadvise and sync_file_range in this case are both trying to put the data in the io scheduler queue (still in the kernel, not on the device). The difference is that fadvise doesn't wait, and sync_file_range does (keep in mind, this is waiting to get in a queue to go to the device, not waiting for the device to write it or even receive it). He indicated that 4096 is a normal number that one might use for the queue size. But on my workstation at home (ubuntu 11.10), the queue is only 128. I bumped it up to 256 and now fadvise is just as fast! This won't be a problem on production systems, but that doesn't help us a lot. People setting up a production system don't care about 6.5 seconds of set-up time anyway. Casual users and developers do (the latter problem can be solved with the --nosync switch, but the former problem is the one we're discussing). So, it looks like fadvise is the right thing to do, but I expect we'll get some widely varying results from actual users. Then again, maybe casual users don't care much about ~10s for initdb anyway? It's a fairly rare operation for everyone except developers. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] renaming domain constraint
Here is a patch for being able to rename constraints of domains. It
goes on top of the previously committed patch for renaming table
constraints.
diff --git a/doc/src/sgml/ref/alter_domain.sgml b/doc/src/sgml/ref/alter_domain.sgml
index 2511a12..c59975a 100644
--- a/doc/src/sgml/ref/alter_domain.sgml
+++ b/doc/src/sgml/ref/alter_domain.sgml
@@ -32,6 +32,8 @@ ALTER DOMAIN replaceable class=PARAMETERname/replaceable
ALTER DOMAIN replaceable class=PARAMETERname/replaceable
DROP CONSTRAINT [ IF EXISTS ] replaceable class=PARAMETERconstraint_name/replaceable [ RESTRICT | CASCADE ]
ALTER DOMAIN replaceable class=PARAMETERname/replaceable
+ RENAME CONSTRAINT replaceable class=PARAMETERconstraint_name/replaceable TO replaceable class=PARAMETERnew_constraint_name/replaceable
+ALTER DOMAIN replaceable class=PARAMETERname/replaceable
VALIDATE CONSTRAINT replaceable class=PARAMETERconstraint_name/replaceable
ALTER DOMAIN replaceable class=PARAMETERname/replaceable
OWNER TO replaceable class=PARAMETERnew_owner/replaceable
@@ -103,6 +105,15 @@ ALTER DOMAIN replaceable class=PARAMETERname/replaceable
/varlistentry
varlistentry
+termRENAME CONSTRAINT/term
+listitem
+ para
+ This form changes the name of a constraint on a domain.
+ /para
+/listitem
+ /varlistentry
+
+ varlistentry
termVALIDATE CONSTRAINT/term
listitem
para
@@ -182,7 +193,7 @@ ALTER DOMAIN replaceable class=PARAMETERname/replaceable
termreplaceable class=PARAMETERconstraint_name/replaceable/term
listitem
para
-Name of an existing constraint to drop.
+Name of an existing constraint to drop or rename.
/para
/listitem
/varlistentry
@@ -226,6 +237,15 @@ ALTER DOMAIN replaceable class=PARAMETERname/replaceable
/varlistentry
varlistentry
+ termreplaceable class=PARAMETERnew_constraint_name/replaceable/term
+ listitem
+ para
+The new name for the constraint.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termreplaceable class=PARAMETERnew_owner/replaceable/term
listitem
para
@@ -289,6 +309,13 @@ ALTER DOMAIN zipcode DROP CONSTRAINT zipchk;
/para
para
+ To rename a check constraint on a domain:
+programlisting
+ALTER DOMAIN zipcode RENAME CONSTRAINT zipchk TO zip_check;
+/programlisting
+ /para
+
+ para
To move the domain into a different schema:
programlisting
ALTER DOMAIN zipcode SET SCHEMA customers;
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index e6e0347..08de88b 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -753,7 +753,7 @@ get_object_address_relobject(ObjectType objtype, List *objname,
case OBJECT_CONSTRAINT:
address.classId = ConstraintRelationId;
address.objectId =
- get_constraint_oid(reloid, depname, missing_ok);
+ get_constraint_oid(reloid, InvalidOid, depname, missing_ok);
address.objectSubId = 0;
break;
default:
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 342cf75..4377207 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -737,17 +737,20 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
/*
* get_constraint_oid
- * Find a constraint on the specified relation with the specified name.
+ * Find a constraint on the specified relation or domain with the specified name.
* Returns constraint's OID.
*/
Oid
-get_constraint_oid(Oid relid, const char *conname, bool missing_ok)
+get_constraint_oid(Oid relid, Oid typid, const char *conname, bool missing_ok)
{
Relation pg_constraint;
HeapTuple tuple;
SysScanDesc scan;
ScanKeyData skey[1];
Oid conOid = InvalidOid;
+ Oid indexId;
+
+ AssertArg(!relid || !typid);
/*
* Fetch the constraint tuple from pg_constraint. There may be more than
@@ -756,12 +759,24 @@ get_constraint_oid(Oid relid, const char *conname, bool missing_ok)
*/
pg_constraint = heap_open(ConstraintRelationId, AccessShareLock);
- ScanKeyInit(skey[0],
-Anum_pg_constraint_conrelid,
-BTEqualStrategyNumber, F_OIDEQ,
-ObjectIdGetDatum(relid));
+ if (relid)
+ {
+ ScanKeyInit(skey[0],
+ Anum_pg_constraint_conrelid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(relid));
+ indexId = ConstraintRelidIndexId;
+ }
+ else
+ {
+ ScanKeyInit(skey[0],
+ Anum_pg_constraint_contypid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(typid));
+ indexId = ConstraintTypidIndexId;
+ }
- scan = systable_beginscan(pg_constraint, ConstraintRelidIndexId, true,
+ scan = systable_beginscan(pg_constraint, indexId, true,
SnapshotNow, 1, skey);
while (HeapTupleIsValid(tuple = systable_getnext(scan)))
@@ -771,10 +786,18 @@ get_constraint_oid(Oid relid, const char *conname, bool missing_ok)
if
Re: [HACKERS] patch: autocomplete for functions
Peter Eisentraut pete...@gmx.net writes: On tor, 2012-03-15 at 16:36 -0300, Alvaro Herrera wrote: Excerpts from Peter Eisentraut's message of jue mar 15 16:25:53 -0300 2012: Isn't that just a subset of what I had proposed? http://archives.postgresql.org/message-id/1328820579.11241.4.ca...@vanquo.pezone.net So do you intend to commit your patch? Well, there was quite a bit of discussion about it, but it appears that most concerns were addressed at the end. So yes, I guess, unless someone wants further discussion. I'm a bit concerned about whether that's actually going to be useful. A quick check shows that in the regression database, the proposed patch produces 3246 possible completions, which suggests that by the time you get down to a unique match you're going to have typed most of the name anyway. BTW, you should at least exclude dropped columns, I think. 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] [BUGS] BUG #6532: pg_upgrade fails on Python stored procedures
On Thu, Mar 15, 2012 at 09:18:36PM +0700, Stuart Bishop wrote: On Thu, Mar 15, 2012 at 9:01 PM, Stuart Bishop stu...@stuartbishop.net wrote: Yes, it is there. I can see the library with the new name of plpython2.so, not the old plpython.so from 8.4. createlang installs the language just fine if I build a cluster and database myself. As expected, symlinking plpython2.so to plpython.so works around things. I have no idea if this work around will cause problems when upgrading the db to PG 9.2+. [ Thread moved to hackers.] Well, it will because, by creating the symlink, you allowed this function to be restored into the new database, and it isn't properly hooked to the plpython language. I wonder if you should just delete it because I believe you already have the right plpython2 helper functions in place. Can you run this query for me in one of the problem databases in the new and/or old cluster and send me the output: SELECT proname,probin FROM pg_proc WHERE probin LIKE '%python%'; What we need is for pg_dumpall to _not_ output those handlers. I did some more digging on this. I am afraid it is related to this problem I discovered on March 5 where the plpython2 helper functions remain after you drop the plpythonu language: http://archives.postgresql.org/pgsql-hackers/2012-03/msg00254.php However, in testing upgrades from 8.4 and 9.0, I don't see those helper functions in the pg_dumpall output, which is very good news. It means this python problem will not hit all users, and hopefully few. Remember, the fix for pg_upgrade in 9.1.3 was to have the shared library file check be adjusted for plpython --- it didn't relate to what pg_dumpall dumps, and as far as I can tell, it is working fine. I did this for testing: PGDATA=/u/pgsql.old/data pgstart sleep 2 aspg /u/pgsql.old/bin/createlang plpythonu test sql -c 'CREATE OR REPLACE FUNCTION pymax (a integer, b integer) RETURNS integer AS $$ if a b: return a return b $$ LANGUAGE plpythonu;' test aspg /u/pgsql.old/bin/psql -c 'DROP LANGUAGE plpythonu CASCADE;' test aspg /u/pgsql.old/bin/psql -c SELECT proname,probin FROM pg_proc WHERE probin LIKE '%python%'; test PGDATA=/u/pgsql.old/data pgstop The SELECT outputs two row from pg_proc: proname | probin -+-- plpython_call_handler | $libdir/plpython plpython_inline_handler | $libdir/plpython (2 rows) showing that even with the plpython language gone, the handler functions are still here. However, those functions do _not_ appear in the pg_dumpall --binary-upgrade --schema-only output, unlike what you are seeing. What the reporter from March 5 and you are seeing are cases where the support functions are being output, which triggers the pg_upgrade failure because the shared library was renamed. For the March 5 reporter, they actually removed plpython, but still had the handlers, and the handlers were being dumped by pg_dumpall. The big question is why do the handlers sometimes get dumped, and sometimes not. The good news is that my testing shows that they are often _not_ dumped, and pg_upgrade works fine. This the query pg_dumpall is using: SELECT tableoid, oid, proname, prolang, pronargs, proargtypes, prorettype, proacl, pronamespace, (SELECT rolname FROM pg_catalog. pg_roles WHERE oid = proowner) AS rolname FROM pg_proc p WHERE NOT proisagg AND (pronamespace != (SELECT oid FROM pg_namespace WHERE nspname = 'pg _catalog')); and I don't get any output running it on my old cluster. Do you get rows output? Specifically, is your handler not in the pg_catalog schema? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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 #6532: pg_upgrade fails on Python stored procedures
On Fri, Mar 16, 2012 at 03:10:05PM +0700, Stuart Bishop wrote: I have repeatedly upgraded from 9.0.X to 9.1.3 and am seeing no failures. The big question is what are you doing that is causing the plpython_call_handler() function to be dumped? That is an internal function. What is your old PG version? I tested 8.4 and also could not get the failure you see either. This database schema began its life on PostgreSQL 7.4 over 8 years ago, so there may well be something unexpected lurking in there. [ Email moved to hackers, and trimmed.] That might be the cause --- see my posting asking for details. I can reproduce the error with the attached schema. It was created using 8.4's pg_dump. If I create a fresh 8.4 cluster and restore this, pg_dump and pg_dumpall spit out the plpython_call_handler statements. I think I've stripped out everything in there not in core or contrib. Thanks. Yes, this actually does show the cause: -- -- Name: plpgsql_call_handler(); Type: FUNCTION; Schema: public; Owner: - -- CREATE FUNCTION plpgsql_call_handler() RETURNS language_handler LANGUAGE c AS '$libdir/plpgsql', 'plpgsql_call_handler'; -- -- Name: plpython_call_handler(); Type: FUNCTION; Schema: public; Owner: - --** CREATE FUNCTION plpython_call_handler() RETURNS language_handler LANGUAGE c AS '$libdir/plpython', 'plpython_call_handler'; Notice these are all in the public schema, which is causing pg_dumpall to output them, and the rename of plpython is causing the failure. Do you have these functions also in the pg_catalog schema? If so, they must be dead functions left over from an old release of Postgres. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] foreign key locks, 2nd attempt
On Fri, Mar 16, 2012 at 10:40:01AM -0300, Alvaro Herrera wrote: Excerpts from Alvaro Herrera's message of vie mar 16 10:36:11 -0300 2012: Now I am confused. Where do you see the word hint used by HEAP_XMAX_EXCL_LOCK and HEAP_XMAX_SHARED_LOCK. These are tuple infomask bits, not hints, meaning they are not optional or there just for performance. Okay, I think this is just a case of confusing terminology. I have always assumed (because I have not seen any evidence to the contrary) that anything in t_infomask and t_infomask2 is a hint bit -- regardless of it being actually a hint or something with a stronger significance. Maybe this is just my mistake. I see in http://wiki.postgresql.org/wiki/Hint_Bits that we only call the COMMITTED/INVALID infomask bits hints. I think it's easy enough to correct the README to call them infomask bits rather than hints .. I'll go do that. OK, thanks. I only brought it up so people would not be confused by thinking these were optional pieces of information, and that the real information is stored somewhere else. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] foreign key locks, 2nd attempt
On Fri, Mar 16, 2012 at 10:08:07AM -0400, Robert Haas wrote: On Thu, Mar 15, 2012 at 11:09 PM, Bruce Momjian br...@momjian.us wrote: On Tue, Mar 13, 2012 at 01:46:24PM -0400, Robert Haas wrote: On Mon, Mar 12, 2012 at 3:28 PM, Simon Riggs si...@2ndquadrant.com wrote: I agree with you that some worst case performance tests should be done. Could you please say what you think the worst cases would be, so those can be tested? That would avoid wasting time or getting anything backwards. I've thought about this some and here's what I've come up with so far: I question whether we are in a position to do the testing necessary to commit this for 9.2. Is anyone even working on testing it? No one I know of. I am just trying to set expectations that this still has a long way to go. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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 assumptions with low LIMITs
I have a query where the LIMIT clause is incorrectly reducing the cost
of the query. This results in queries doing LIMIT m run much longer
when m is small (e.g. 1-3) than if m is larger. (PG 9.0) (And yes,
ANALYZE was run and, no, increasing stats_target for the columns
doesn't help).
Notice that the total predicted cost is a small fraction of the cost
of the IndexScan. It's a small cost because we assume that the LIMIT
reduces the cost of the query in a linear manner, which just ain't so.
Limit (cost=0.00..8320.49 rows=25 width=28) (actual
time=0.024..239.044 rows=25 loops=1)
- Index Scan Backward using blah_pkey on blah
(cost=0.00..4112652.70 rows=12357 width=28) (actual
time=0.022..239.009 rows=25 loops=1)
Index Cond: (blah_id = 72572020)
Filter: (user_id = ANY ('{list}'::integer[]))
What we are assuming is that if doing the whole scan would reveal N
rows, then we only need to do m/N fraction of the scan per output row.
So if the Limit is very small then this means that the cost of this
plan looks really low. Low enough that it beats the plan you might
expect to see, which is an IndexScan or a BitMapIndexScan on user_id.
This inappropriate assumption is causing bad plans on the two worst
query types on this large database, so its not just an idle annoyance.
I've also seen this before in other cases at other release levels. I'm
not reporting this because I need help finding a solution, but because
its a clear bug. So its not a topic for the performance list.
Other examples
Limit 1-3Total runtime: 3394.080 ms
example plan
Limit (cost=0.00..1719.89 rows=2 width=1757) (actual
time=3394.000..3394.000 rows=0 loops=1)
- Nested Loop (cost=0.00..769649.42 rows=895 width=1757) (actual
time=3393.998..3393.998 rows=0 loops=1)
- Seq Scan on user (cost=0.00..763051.97 rows=895
width=1607) (actual time=3393.996..3393.996 rows=0 loops=1)
Filter: ((user_id ALL ('{...about 10
users...}'::integer[])) AND (thing_id = ANY ('{array of
integers}'::bigint[])))
- Index Scan using auth_user_pkey on auth_user
(cost=0.00..7.36 rows=1 width=150) (never executed)
Index Cond: (id = user.user_id)
Limit 4+ Total runtime: 2.132 ms
Limit (cost=3052.13..3100.82 rows=4 width=1757) (actual
time=2.053..2.053 rows=0 loops=1)
- Nested Loop (cost=3052.13..13946.44 rows=895 width=1757)
(actual time=2.050..2.050 rows=0 loops=1)
- Bitmap Heap Scan on user (cost=3052.13..7348.98 rows=895
width=1607) (actual time=2.048..2.048 rows=0 loops=1)
Recheck Cond: (thing_id = ANY ('{...lots...}'::bigint[]))
Filter: (user_id ALL ('{...~10 users...}'::integer[]))
- Bitmap Index Scan on user_thing_id
(cost=0.00..3051.90 rows=895 width=0) (actual time=2.026..2.026 rows=6
loops=1)
Index Cond: (thing_id = ANY ('{...lots...}'::bigint[]))
- Index Scan using auth_user_pkey on auth_user
(cost=0.00..7.36 rows=1 width=150) (never executed)
Index Cond: (id = user.user_id)
2000x slower is enough for me to call this a bug, rather than
euphemistically saying this is sub-optimal.
So there are a few problems:
1. We assume that all values exist within the table. There is no
measure of sparsity. Any value between min and max is assumed to exist
and is assigned the averaged selectivity for a non-MFV.
2. We assume that if values do exist that they have rows uniformly
distributed across the whole table like rungs on a ladder. So the
fewer rows you want the cheaper it is to access them. This mistake is
made any time we propagate the LIMIT clause onto a SeqScan.
3. We take no account of the potential for error in the plan. The plan
chosen is very risky, but has a lower cost. It would be better to
favour less risky plans for most cases.
So (1) leads us to make an overestimate of the selectivity, then we
use (2) to deduce that the overall cost is therefore incredibly low.
So the worst case selectivity actually leads us to making a
best-case assumption: that rows are frequent and also uniformly
distributed.
In terms of proposed solutions, (2) is the only one that seems able to
be altered with any ease. If we have a reasonably large selectivity,
its OK to assume that we don't have to scan much of a table before we
find a matching row. But taking that assumption to extremes causes
these bad plans.
Any time we apply a LIMIT clause to a plan with a SeqScan or
unqualified IndexScan, we shouldn't assume the scan will do less than
say 10% of the table. It might, but its an unsafe assumption because
as the selectivity decreases so does the safety of the assumption that
rows are uniformly distributed. So basically put a clamp on the
ability of this assumption to scale downwards when m/N is very small.
We could also improve on (1) for some data types. Some measure of
sparsity could easily be gained by taking NDistinct/(Max - Min) for
integer types. That is useful because integers are
Re: [HACKERS] foreign key locks, 2nd attempt
Excerpts from Bruce Momjian's message of vie mar 16 15:22:05 -0300 2012: On Fri, Mar 16, 2012 at 10:08:07AM -0400, Robert Haas wrote: On Thu, Mar 15, 2012 at 11:09 PM, Bruce Momjian br...@momjian.us wrote: On Tue, Mar 13, 2012 at 01:46:24PM -0400, Robert Haas wrote: On Mon, Mar 12, 2012 at 3:28 PM, Simon Riggs si...@2ndquadrant.com wrote: I agree with you that some worst case performance tests should be done. Could you please say what you think the worst cases would be, so those can be tested? That would avoid wasting time or getting anything backwards. I've thought about this some and here's what I've come up with so far: I question whether we are in a position to do the testing necessary to commit this for 9.2. Is anyone even working on testing it? No one I know of. I am just trying to set expectations that this still has a long way to go. A Command Prompt colleague is on it. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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 #6532: pg_upgrade fails on Python stored procedures
On Sat, Mar 17, 2012 at 12:54 AM, Bruce Momjian br...@momjian.us wrote: Well, it will because, by creating the symlink, you allowed this function to be restored into the new database, and it isn't properly hooked to the plpython language. I wonder if you should just delete it because I believe you already have the right plpython2 helper functions in place. Can you run this query for me in one of the problem databases in the new and/or old cluster and send me the output: SELECT proname,probin FROM pg_proc WHERE probin LIKE '%python%'; # SELECT nspname,proname,probin FROM pg_proc,pg_namespace WHERE probin LIKE '%python%' and pg_proc.pronamespace=pg_namespace.oid; nspname |proname| probin +---+-- pg_catalog | plpython_call_handler | $libdir/plpython public | plpython_call_handler | $libdir/plpython (2 rows) I have no idea how I managed to grow the duplicate in the public schema, but this does seem to be the source of the confusion. I might be able to dig out when I grew it from revision control, but I don't think that would help. What we need is for pg_dumpall to _not_ output those handlers. Or pick it up in the check stage and make the user resolve the problem. If I shot myself in the foot in some particularly obtuse way, it might not be sane to bend over backwards making pg_upgrade repair things. -- Stuart Bishop stu...@stuartbishop.net http://www.stuartbishop.net/ -- 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 #6532: pg_upgrade fails on Python stored procedures
On Sat, Mar 17, 2012 at 01:57:29AM +0700, Stuart Bishop wrote: On Sat, Mar 17, 2012 at 12:54 AM, Bruce Momjian br...@momjian.us wrote: Well, it will because, by creating the symlink, you allowed this function to be restored into the new database, and it isn't properly hooked to the plpython language. I wonder if you should just delete it because I believe you already have the right plpython2 helper functions in place. Can you run this query for me in one of the problem databases in the new and/or old cluster and send me the output: SELECT proname,probin FROM pg_proc WHERE probin LIKE '%python%'; # SELECT nspname,proname,probin FROM pg_proc,pg_namespace WHERE probin LIKE '%python%' and pg_proc.pronamespace=pg_namespace.oid; nspname |proname| probin +---+-- pg_catalog | plpython_call_handler | $libdir/plpython public | plpython_call_handler | $libdir/plpython (2 rows) I have no idea how I managed to grow the duplicate in the public schema, but this does seem to be the source of the confusion. I might be able to dig out when I grew it from revision control, but I don't think that would help. Yes, if you delete the public one, you should be fine. If you need CASCADE, then something is wrong because their is some depenency on it. Odds are it might have gotten created before we had full schema support for language or something. The March 5 reporter probably had the same problem, so isn't just you. What we need is for pg_dumpall to _not_ output those handlers. Or pick it up in the check stage and make the user resolve the problem. If I shot myself in the foot in some particularly obtuse way, it might not be sane to bend over backwards making pg_upgrade repair things. I think we need someone to figure out how this happened before we actually adjust anything. At least we know what to advise people now. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Command Triggers, v16
On 16 March 2012 16:26, Tom Lane t...@sss.pgh.pa.us wrote: Dimitri Fontaine dimi...@2ndquadrant.fr writes: Tom Lane t...@sss.pgh.pa.us writes: If you think cmdtrigger isn't a good name maybe you should have picked a different one to start with. Well, I think it's a good internal name. I'm not too sure about exposing it, the only reason why it's a good name is because it's a single not too long word, after all. Not very “SQLish”. I'm putting cmdtrigger as the user visible name in the next version of the patch, if you come up with something potentially more user friendly feel free to suggest. How about commandtrigger or command_trigger? Typing a few more characters in this context doesn't seem like a deal-breaker to me. +1 No objections to either of those suggestions, although I'd lean towards the one without the underscore, not for any technical reason. There is a precedent for a type with an underscore in its name (txid_snapshot) but seems to be the exception. Thom -- 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] Command Triggers, patch v11
Andres Freund and...@anarazel.de writes: [ ctas-01.patch ] I'm starting to look at this now. For a patch that's supposed to de-complicate things, it seems pretty messy :-( One thing I soon found is that it lacks support for EXPLAIN SELECT INTO. That used to work, but now you get regression=# explain select * into foo from tenk1; ERROR: INTO is only allowed on first SELECT of UNION/INTERSECT/EXCEPT LINE 1: explain select * into foo from tenk1; ^ and while fixing the parse analysis for that is probably not too hard, it would still fail to work nicely, since explain.c lacks support for CreateTableAsStmt utility statements. I think we'd have to invent something parallel to ExplainExecuteQuery to support this, and I really doubt that it's worth the trouble. Does anyone have a problem with desupporting the case? Also, it seems to me that the patch is spending way too much effort on trying to exactly duplicate the old error messages for various flavors of INTO not allowed here, and still not succeeding terribly well. I'm inclined to just have a one-size-fits-all message in transformSelectStmt, which will fire if intoClause hasn't been cleared before we get there; any objections? A couple of other cosmetic thoughts: I'm tempted to split the execution support out into a new file, rather than bloat tablecmds.c any further; and I'm wondering if the interface to EXECUTE INTO can't be simplified. (It may have been a mistake to remove the IntoClause in ExecuteStmt.) 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] Command Triggers, patch v11
On Friday, March 16, 2012 09:54:47 PM Tom Lane wrote: Andres Freund and...@anarazel.de writes: [ ctas-01.patch ] I'm starting to look at this now. Great! For a patch that's supposed to de-complicate things, it seems pretty messy :-( Yea. It started out simple but never stopped getting more complicated. Getting rid of the duplication still seems to make sense to me though. One thing I soon found is that it lacks support for EXPLAIN SELECT INTO. That used to work, but now you get regression=# explain select * into foo from tenk1; ERROR: INTO is only allowed on first SELECT of UNION/INTERSECT/EXCEPT LINE 1: explain select * into foo from tenk1; ^ and while fixing the parse analysis for that is probably not too hard, it would still fail to work nicely, since explain.c lacks support for CreateTableAsStmt utility statements.I think we'd have to invent something parallel to ExplainExecuteQuery to support this, and I really doubt that it's worth the trouble. Does anyone have a problem with desupporting the case? I am all for removing it. Also, it seems to me that the patch is spending way too much effort on trying to exactly duplicate the old error messages for various flavors of INTO not allowed here, and still not succeeding terribly well. I'm inclined to just have a one-size-fits-all message in transformSelectStmt, which will fire if intoClause hasn't been cleared before we get there; any objections? I was/am decidedly unhappy about the whole error message stuff. Thats what turned the patch from removing lines to making it bigger than before. I tried to be compatible to make sure I understood what was happening. I am fine with making it one simple error message. A couple of other cosmetic thoughts: I'm tempted to split the execution support out into a new file, rather than bloat tablecmds.c any further; and I'm wondering if the interface to EXECUTE INTO can't be simplified. (It may have been a mistake to remove the IntoClause in ExecuteStmt.) Hm. I vote against keeping the IntoClause stuff in ExecuteStmt. Splitting stuff from tablecmd.c seems sensible though. One more thing I disliked quite a bit was the duplication of the EXECUTE handling. Do you see a way to deduplicate that? If you give me some hints about what to change I am happy to revise the patch myself should you prefer that. 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] Incorrect assumptions with low LIMITs
Simon Riggs si...@2ndquadrant.com writes: 2. We assume that if values do exist that they have rows uniformly distributed across the whole table like rungs on a ladder. Well, yeah. That's sometimes wrong, but not always. In the absence of evidence to the contrary, I think it's a better assumption than most others. Any time we apply a LIMIT clause to a plan with a SeqScan or unqualified IndexScan, we shouldn't assume the scan will do less than say 10% of the table. This is a horrid idea, because it will stop the planner from using fast-start plans in many situations where they are wins. Other ideas welcome. You are not the first person to have run into this type of problem. If it were easily solved by some minor hack, we would have done that long since. The problem is to not throw the baby out with the bathwater. I can see an argument for downgrading the assumed effectiveness of restrictions that are applied as qpquals (ie, not indexquals), but the LIMIT node coster doesn't have easy access to the information needed for that, especially not in situations more complicated than a single-table scan. 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] Command Triggers, patch v11
Andres Freund and...@anarazel.de writes: One more thing I disliked quite a bit was the duplication of the EXECUTE handling. Do you see a way to deduplicate that? Yeah, that's what's bugging me, too. I think a chunk of the problem is that you're insisting on having control come back to CreateTableAs() to perform the table creation. I'm thinking that if the table creation were to be moved into the tuple receiver's startup routine, we could avoid needing to get control back between ExecutorStartup and ExecutorRun, and then all that would be required would be to call ExecuteQuery with a different DestReceiver. 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] Command Triggers, patch v11
On Friday, March 16, 2012 10:31:57 PM Tom Lane wrote: Andres Freund and...@anarazel.de writes: One more thing I disliked quite a bit was the duplication of the EXECUTE handling. Do you see a way to deduplicate that? Yeah, that's what's bugging me, too. I think a chunk of the problem is that you're insisting on having control come back to CreateTableAs() to perform the table creation. I'm thinking that if the table creation were to be moved into the tuple receiver's startup routine, we could avoid needing to get control back between ExecutorStartup and ExecutorRun, and then all that would be required would be to call ExecuteQuery with a different DestReceiver. Hm. I seriously dislike doing that in the receiver. I can't really point out why though. Unsurprisingly I like getting the control back to CreateTableAs... 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] Incorrect assumptions with low LIMITs
On Fri, 2012-03-16 at 18:25 +, Simon Riggs wrote: Any time we apply a LIMIT clause to a plan with a SeqScan or unqualified IndexScan, we shouldn't assume the scan will do less than say 10% of the table. It might, but its an unsafe assumption because as the selectivity decreases so does the safety of the assumption that rows are uniformly distributed. Just trying to follow along. You mean as the selectivity _increases_ the safety of the assumption that the rows are uniformly distributed decreases, right? Regards, Jeff Davis -- 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] Command Triggers, patch v11
Andres Freund and...@anarazel.de writes: On Friday, March 16, 2012 10:31:57 PM Tom Lane wrote: I'm thinking that if the table creation were to be moved into the tuple receiver's startup routine, we could avoid needing to get control back between ExecutorStartup and ExecutorRun, and then all that would be required would be to call ExecuteQuery with a different DestReceiver. Hm. I seriously dislike doing that in the receiver. I can't really point out why though. Unsurprisingly I like getting the control back to CreateTableAs... I don't see the argument. Receiver startup functions are intended to do exactly this type of thing. I'd be okay with leaving it in CreateTableAs if it didn't contort the code to do so, but what we have here is precisely that we've got to contort the interface with prepare.c to do it that way. (It also occurs to me that moving this work into the DestReceiver might make things self-contained enough that we could continue to support EXPLAIN SELECT INTO with not an undue amount of pain.) Something else I just came across is that there are assorted places that are aware that ExplainStmt contains a Query, eg setrefs.c, plancache.c, and those have got to treat CreateTableAsStmt similarly. We could just add more code in each of those places. I'm wondering though if it would be a good idea to invent an abstraction layer, to wit a utility.c function along the lines of Query *UtilityContainsQuery(Node *parsetree), which would centralize the knowledge of exactly which utility statements are like this and how to dig the Query out of them. It's only marginally attractive unless there's a foreseeable reason why we'd someday have more than two of them; but on the other hand, just formalizing the concept that some utility statements are like this might be a good thing. (Actually, as I type this I wonder whether COPY (SELECT ...) isn't a member of this class too, and whether we don't have bugs from the fact that it's not being treated like EXPLAIN.) Thoughts? 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] Incorrect assumptions with low LIMITs
On Fri, Mar 16, 2012 at 9:39 PM, Jeff Davis pg...@j-davis.com wrote: On Fri, 2012-03-16 at 18:25 +, Simon Riggs wrote: Any time we apply a LIMIT clause to a plan with a SeqScan or unqualified IndexScan, we shouldn't assume the scan will do less than say 10% of the table. It might, but its an unsafe assumption because as the selectivity decreases so does the safety of the assumption that rows are uniformly distributed. Just trying to follow along. You mean as the selectivity _increases_ the safety of the assumption that the rows are uniformly distributed decreases, right? Selectivity meaning the value between 0 and 1 that describes, in the planner, the fraction of rows we will get back from a scan. 1.0 means 100% of rows. When selectivity is low, that means very few rows will come back. I think you are using high selectivity as meaning returns few of the rows, so you understand me, but just flip the meaning of the words. When you have lots of rows, its a good assumption they are spread out and a scan will find some of them quickly. When you have very few rows, assuming they are evenly spaced is just weird. Clumpiness of some kind seems much more likely. Much more easily understood if the values are dates, for example. Given the estimated number of rows is deliberately a worst case (i,e. high), that sounds like the scan will work. Yet the reality is that doing the scan is incredibly costly when those assumptions break, which they do, often. Especially when the values don't exist at all because the table is sparse. -- Simon Riggs 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] Storage Manager crash at mdwrite()
On Thu, Mar 15, 2012 at 7:58 PM, Jeff Davis pg...@j-davis.com wrote: On Thu, 2012-03-15 at 19:36 -0400, Tareq Aljabban wrote: When configuring postgreSQL, I'm adding the libraries needed to run HDFS C API (libhdfs). From the information below, it looks like C++. ./configure --prefix=/diskless/myUser/Workspace/EclipseWS1/pgsql --enable-depend --enable-cassert --enable-debug CFLAGS=$CFLAGS -I/diskless/myUser/Workspace/HDFS_Append/hdfs/src/c++/libhdfs -I/usr/lib/jvm/default-java/include LDFLAGS=$LDFLAGS -L/diskless/myUser/Workspace/HDFS_Append/hdfs/src/c++/libhdfs -L/diskless/myUser/Workspace/HDFS_Append/build/c++/Linux-i386-32/lib -L/usr/lib/jvm/default-java/jre/lib/i386/server -ljvm -lhdfs I have done lots of changes so far on how the storage manager works. In fact, I changed smgr.c so instead of calling regular md.c functions, that it would call my functions . For simplicity, you can say that whenever mdwrite() was supposed to be called, another function is also called beside it. so now what is being called is: mdwrite(param1, param2, buffer, ); hdfswrite(param1, param2, buffer, ); where hdfswrite() is the function where I write the buffer to HDFS. I changed hdfswrite() so it will always write only the same (dummy) buffer down to HDFS storage. Let's say dummyBufferFile. After writing this file 3 times to HDFS, I'm getting the message that I've shown in my first email. The same hdfswrite() code works without any issues when I run it in a separate application. Hope it's clear now. Well, it's clear that there's a lot going on ;) In other words, is there a reason you think that these would all work together nicely? I don't know the specifics, but I understand there are numerous perils to linking complex C++ code into complex C code, particularly around exception handling. Look in the archives for previous discussions around C++. Thanks for your response Jeff.. It's true that libhdfs code resides under the c++ folder, but in all of the documentation, libhdfs is referred to as the C interface of HDFS. Now what you're saying makes sense.. that nothing guarantees this will work well.. but in the phase of initDB, the function calls are done nicely without any exceptions.. when starting the postmaster, something wrong happens after 3 calls to libhdfs.. that's what I'm confused about.. What it's the difference between the two processes (initDB, start postmaster), that might cause this crash to happen? Regards
Re: [HACKERS] Command Triggers, patch v11
On Friday, March 16, 2012 10:52:55 PM Tom Lane wrote: Andres Freund and...@anarazel.de writes: On Friday, March 16, 2012 10:31:57 PM Tom Lane wrote: I'm thinking that if the table creation were to be moved into the tuple receiver's startup routine, we could avoid needing to get control back between ExecutorStartup and ExecutorRun, and then all that would be required would be to call ExecuteQuery with a different DestReceiver. Hm. I seriously dislike doing that in the receiver. I can't really point out why though. Unsurprisingly I like getting the control back to CreateTableAs... I don't see the argument. Receiver startup functions are intended to do exactly this type of thing. I'd be okay with leaving it in CreateTableAs if it didn't contort the code to do so, but what we have here is precisely that we've got to contort the interface with prepare.c to do it that way. Hm. (It also occurs to me that moving this work into the DestReceiver might make things self-contained enough that we could continue to support EXPLAIN SELECT INTO with not an undue amount of pain.) Something else I just came across is that there are assorted places that are aware that ExplainStmt contains a Query, eg setrefs.c, plancache.c, and those have got to treat CreateTableAsStmt similarly. Hm. Is that so? As implemented in my version the planner just sees a plain statement instead of a utility statement. Am I missing something? I am not even sure why the planner ever needs to see ExplainStmts? Protocol level prepares? Shouldn't those top level nodes be simply removed once? We could just add more code in each of those places. I'm wondering though if it would be a good idea to invent an abstraction layer, to wit a utility.c function along the lines of Query *UtilityContainsQuery(Node *parsetree), which would centralize the knowledge of exactly which utility statements are like this and how to dig the Query out of them. It's only marginally attractive unless there's a foreseeable reason why we'd someday have more than two of them; but on the other hand, just formalizing the concept that some utility statements are like this might be a good thing. If its really necessary to do that I think that would be a good idea. Alone the increased greppablility/readablility seems to be worth it. (Actually, as I type this I wonder whether COPY (SELECT ...) isn't a member of this class too, and whether we don't have bugs from the fact that it's not being treated like EXPLAIN.) Thoughts? Hm. On a first glance the planner also never sees the content of a CopyStmt... 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] pg_terminate_backend for same-role
On Thu, Mar 15, 2012 at 04:14:03PM -0700, Daniel Farina wrote: Parallel to pg_cancel_backend, it'd be nice to allow the user to just outright kill a backend that they own (politely, with a SIGTERM), aborting any transactions in progress, including the idle transaction, and closing the socket. +1 I imagine the problem is a race condition whereby a pid might be reused by another process owned by another user (doesn't that also affect pg_cancel_backend?). Shall we just do everything using the MyCancelKey (which I think could just be called SessionKey, SessionSecret, or even just Session) as to ensure we have no case of mistaken identity? Or does that end up being problematic? No, I think the hazard you identify here is orthogonal to the question of when to authorize pg_terminate_backend(). As you note downthread, protocol-level cancellations available in released versions already exhibit this hazard. I wouldn't mind a clean fix for this, but it's an independent subject. Here I discussed a hazard specific to allowing pg_terminate_backend(): http://archives.postgresql.org/message-id/20110602045955.gc8...@tornado.gateway.2wire.net To summarize, user code can trap SIGINT cancellations, but it cannot trap SIGTERM terminations. If a backend is executing a SECURITY DEFINER function when another backend of the same role calls pg_terminate_backend() thereon, the pg_terminate_backend() caller could achieve something he cannot achieve in PostgreSQL 9.1. I vote that this is an acceptable loss. Thanks, nm -- 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_upgrade and statistics
On Thu, Mar 15, 2012 at 11:46:04AM -0400, Bruce Momjian wrote: On Thu, Mar 15, 2012 at 11:15:42AM -0400, Andrew Dunstan wrote: You're not the only person who could do that. I don't think this is all down to you. It should just be understood that if the stats format is changed, adjusting pg_upgrade needs to be part of the change. When we modified how enums worked, we adjusted pg_upgrade at the same time. That sort of thing seems totally reasonable to me. I haven't looked at it, but I'm wondering how hard it is going to be in practice? Well, the reason I am not recommending migration is because the work will _not_ fall on me, but rather on the larger community of developers, who might or might not care about pg_upgrade. (I am concerned about pg_upgrade retarding development progress.) We are already telling developers not to change the binary storage format without considering pg_upgrade --- do we want to do the same for optimizer statistics? Does the optimizer statistics format change more frequently than the storage format? I think the answer is yes. Does it change too frequently to require migration work? I don't know the answer to that, partly because I would not be the one doing the work. Also, considering we are on the last 9.2 commit-fest, I doubt we can get something working for statistics migration for 9.2, I think the incremental statistics build approach is our only way to improve this for 9.2, and frankly, 9.1 users can also use the script once I post it. FYI, I have not received a huge number of complaints about the old analyze recommendation --- a few, but not a flood. The incremental build approach might be good enough. My wild guess is that even if we migrated all statistics, the migrated statistics will still not have any improvements we have made in statistics gathering, meaning there will still be some kind of analyze process necessary, hopefully just on the affected tables --- that would be shorter, but perhaps not shorter enough to warrant the work in migrating the statistics. I am attaching the updated script and script output. Please, don't think I am ungrateful for the offers of help in migrating statistics. I just remember how complex the discussion was when we modified the enum improvements to allow pg_upgrade, and how complex the checksum discussion was related to pg_upgrade. Applied to git head for PG 9.2. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_upgrade and pg_config dependency
Àlvaro told me he got a Spanish-language report that pg_upgrade
failed because it required pg_config, and pg_config is only supplied
with the devel packages.
I initially thought that it was a packaging problem, but I later
realized the pg_config is mostly developer settings, and that using
pg_config was not getting any change to libdir by dynamic_library_path
in postgresql.conf, and that I should just merge the pg_upgrade_support
detection code into the existing shared library detection LOAD code I
already had.
This avoids the pg_config dependency, works better for libdir, and
reduces the amount of code.
Patch attached. Should this be backpatched to PG 9.1; I think so.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 9fbfd40..03382d9
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** static void check_is_super_user(ClusterI
*** 20,26
static void check_for_prepared_transactions(ClusterInfo *cluster);
static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
- static void check_for_support_lib(ClusterInfo *cluster);
static void get_bin_version(ClusterInfo *cluster);
#ifndef WIN32
--- 20,25
*** check_cluster_versions(void)
*** 270,277
void
check_cluster_compatibility(bool live_check)
{
- check_for_support_lib(new_cluster);
-
/* get/check pg_control data of servers */
get_control_data(old_cluster, live_check);
get_control_data(new_cluster, false);
--- 269,274
*** check_for_reg_data_type_usage(ClusterInf
*** 841,885
}
- /*
- * Test pg_upgrade_support.so is in the proper place. We cannot copy it
- * ourselves because install directories are typically root-owned.
- */
- static void
- check_for_support_lib(ClusterInfo *cluster)
- {
- char cmd[MAXPGPATH];
- char libdir[MAX_STRING];
- char libfile[MAXPGPATH];
- FILE *lib_test;
- FILE *output;
-
- snprintf(cmd, sizeof(cmd), \%s/pg_config\ --pkglibdir, cluster-bindir);
-
- if ((output = popen(cmd, r)) == NULL ||
- fgets(libdir, sizeof(libdir), output) == NULL)
- pg_log(PG_FATAL, Could not get pkglibdir data using %s: %s\n,
- cmd, getErrorText(errno));
-
-
- pclose(output);
-
- /* Remove trailing newline */
- if (strchr(libdir, '\n') != NULL)
- *strchr(libdir, '\n') = '\0';
-
- snprintf(libfile, sizeof(libfile), %s/pg_upgrade_support%s, libdir,
- DLSUFFIX);
-
- if ((lib_test = fopen(libfile, r)) == NULL)
- pg_log(PG_FATAL,
- The pg_upgrade_support module must be created and installed in the %s cluster.\n,
- CLUSTER_NAME(cluster));
-
- fclose(lib_test);
- }
-
-
static void
get_bin_version(ClusterInfo *cluster)
{
--- 838,843
diff --git a/contrib/pg_upgrade/function.c b/contrib/pg_upgrade/function.c
new file mode 100644
index 3225039..fe8fb40
*** a/contrib/pg_upgrade/function.c
--- b/contrib/pg_upgrade/function.c
***
*** 13,18
--- 13,19
#include access/transam.h
+ #define PG_UPGRADE_SUPPORT $libdir/pg_upgrade_support
/*
* install_support_functions_in_new_db()
*** get_loadable_libraries(void)
*** 154,170
PQfinish(conn);
}
/* Allocate what's certainly enough space */
! if (totaltups 0)
! os_info.libraries = (char **) pg_malloc(totaltups * sizeof(char *));
! else
! os_info.libraries = NULL;
/*
* Now remove duplicates across DBs. This is pretty inefficient code, but
* there probably aren't enough entries to matter.
*/
totaltups = 0;
for (dbnum = 0; dbnum old_cluster.dbarr.ndbs; dbnum++)
{
--- 155,171
PQfinish(conn);
}
+ totaltups++; /* reserve for pg_upgrade_support */
+
/* Allocate what's certainly enough space */
! os_info.libraries = (char **) pg_malloc(totaltups * sizeof(char *));
/*
* Now remove duplicates across DBs. This is pretty inefficient code, but
* there probably aren't enough entries to matter.
*/
totaltups = 0;
+ os_info.libraries[totaltups++] = pg_strdup(PG_UPGRADE_SUPPORT);
for (dbnum = 0; dbnum old_cluster.dbarr.ndbs; dbnum++)
{
*** check_loadable_libraries(void)
*** 256,261
--- 257,268
if (PQresultStatus(res) != PGRES_COMMAND_OK)
{
found = true;
+
+ /* exit and report missing support library with special message */
+ if (strcmp(lib, PG_UPGRADE_SUPPORT) == 0)
+ pg_log(PG_FATAL,
+ The pg_upgrade_support module must be created and installed in the new cluster.\n);
+
if (script == NULL (script = fopen_priv(output_path, w)) == NULL)
pg_log(PG_FATAL, Could not open file \%s\: %s\n,
output_path, getErrorText(errno));
--
Sent via
Re: [HACKERS] pg_upgrade and pg_config dependency
Excerpts from Bruce Momjian's message of vie mar 16 20:06:28 -0300 2012: Àlvaro told me he got a Spanish-language report that pg_upgrade failed because it required pg_config, and pg_config is only supplied with the devel packages. I initially thought that it was a packaging problem, but I later realized the pg_config is mostly developer settings, and that using pg_config was not getting any change to libdir by dynamic_library_path in postgresql.conf, and that I should just merge the pg_upgrade_support detection code into the existing shared library detection LOAD code I already had. This avoids the pg_config dependency, works better for libdir, and reduces the amount of code. Bruce also found a reference to this old bug report: http://archives.postgresql.org/pgsql-bugs/2011-12/msg00254.php This includes a link to a Debian bug report by Peter. Patch attached. Should this be backpatched to PG 9.1; I think so. +1 -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Storage Manager crash at mdwrite()
On Fri, 2012-03-16 at 18:02 -0400, Tareq Aljabban wrote: Thanks for your response Jeff.. It's true that libhdfs code resides under the c++ folder, but in all of the documentation, libhdfs is referred to as the C interface of HDFS. Now what you're saying makes sense.. that nothing guarantees this will work well.. but in the phase of initDB, the function calls are done nicely without any exceptions.. when starting the postmaster, something wrong happens after 3 calls to libhdfs.. that's what I'm confused about.. What it's the difference between the two processes (initDB, start postmaster), that might cause this crash to happen? There is a lot of difference between those two. In particular, it looks like the problem you are seeing is coming from the background writer, which is not running during initdb. I think the next step would be for you to compile in debug mode with -O0, and attach to the bgwriter process with gdb, and step through it. Then, you can see the exact path which causes the bgwriter to exit, and that will give you a better idea where the problem is. Regards, Jeff Davis -- 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_terminate_backend for same-role
On Fri, Mar 16, 2012 at 3:42 PM, Noah Misch n...@leadboat.com wrote: On Thu, Mar 15, 2012 at 04:14:03PM -0700, Daniel Farina wrote: Parallel to pg_cancel_backend, it'd be nice to allow the user to just outright kill a backend that they own (politely, with a SIGTERM), aborting any transactions in progress, including the idle transaction, and closing the socket. +1 I imagine the problem is a race condition whereby a pid might be reused by another process owned by another user (doesn't that also affect pg_cancel_backend?). Shall we just do everything using the MyCancelKey (which I think could just be called SessionKey, SessionSecret, or even just Session) as to ensure we have no case of mistaken identity? Or does that end up being problematic? No, I think the hazard you identify here is orthogonal to the question of when to authorize pg_terminate_backend(). As you note downthread, protocol-level cancellations available in released versions already exhibit this hazard. I wouldn't mind a clean fix for this, but it's an independent subject. Hmm. Well, here's a patch that implements exactly that, I think, similar to the one posted to this thread, but not using BackendIds, but rather the newly-introduced SessionId. Would appreciate comments. Because an out-of-band signaling method has been integrated more complex behaviors -- such as closing the TERM-against-SECURITY-DEFINER-FUNCTION hazard -- can be addressed. For now I've only attempted to solve the problem of backend ambiguity, which basically necessitated out-of-line information transfer as per the usual means. Here I discussed a hazard specific to allowing pg_terminate_backend(): http://archives.postgresql.org/message-id/20110602045955.gc8...@tornado.gateway.2wire.net To summarize, user code can trap SIGINT cancellations, but it cannot trap SIGTERM terminations. If a backend is executing a SECURITY DEFINER function when another backend of the same role calls pg_terminate_backend() thereon, the pg_terminate_backend() caller could achieve something he cannot achieve in PostgreSQL 9.1. I vote that this is an acceptable loss. I'll throw out a patch that just lets this hazard exist and see what happens, although it is obsoleted/incompatible with the one already attached. -- fdr Implement-race-free-sql-originated-backend-cancellation-v2.patch.gz Description: GNU Zip compressed 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] pg_terminate_backend for same-role
On Fri, Mar 16, 2012 at 4:42 PM, Daniel Farina dan...@heroku.com wrote: Hmm. Well, here's a patch that implements exactly that, I think, That version had some screws loose due to some editor snafus. Hopefully all better. -- fdr Implement-race-free-sql-originated-backend-cancellation-v3.patch.gz Description: GNU Zip compressed 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] Storage Manager crash at mdwrite()
On Fri, Mar 16, 2012 at 11:29 PM, Jeff Davis pg...@j-davis.com wrote: There is a lot of difference between those two. In particular, it looks like the problem you are seeing is coming from the background writer, which is not running during initdb. The difference that comes to mind is that the postmaster forks. If the library opens any connections prior to forking and then uses them after forking that would work at first but it would get confused quickly once more than one backend tries to use the same connection. The data being sent would all be mixed together and they would see responses to requests other processes sent. You need to ensure that any network connections are opened up *after* the new processes are forked. There are other things going on that could cause problems. initdb probably doesn't deal with many errors so it might not be casing any longjmps that happen when Postgres handles errors. I suspect it doesn't create any temporary files, etc. There's also a long history of threads not playing well with Postgres. I think that's overblown and I believe they should work fine. Most of it was caused by a bug in an old version of libc. But you do have to ensure that the other threads don't call any Postgres functions because the Postgres code is very much not thread-aware. -- greg -- 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] Support for foreign keys with arrays
[used followup EACH-foreign-key.v4b.patch.bz2 for review]
On Wed, Mar 14, 2012 at 07:03:08PM +0100, Marco Nenciarini wrote:
please find attached v4 of the EACH Foreign Key patch (formerly known
also as Foreign Key Array).
On Fri, Feb 24, 2012 at 09:01:35PM -0500, Noah Misch wrote:
We can use multi-dimensional arrays as well as referencing columns. In
that case though, ON DELETE EACH CASCADE will behave like ON DELETE EACH
SET NULL. This is a safe way of implementing the action.
We have some ideas on how to implement this, but we feel it is better to
limit the behaviour for now.
This still feels awfully unprincipled to me. How about just throwing an
error
when we need to remove an element from a multidimensional array?
Essentially,
have ON DELETE EACH CASCADE downgrade to ON DELETE RESTRICT when it
encounters
a multidimensional array. That's strictly less code than what you have now,
and it keeps our options open. We can always change from error to set-null
later, but we can't so readily change from set-null to anything else.
That seems reasonable to me. Implemented and documented.
I like the semantics now. You implemented this by doing two scans per PK
delete, the first to detect multidimensional dependants of the PK row and the
second to fix 1-dimensional dependants. We don't require an index to support
the scan, so this can mean two seq scans. Currently, the only benefit to
doing it this way is a change of error message. Here is the current error
message when we would need to remove a multidimensional array element:
ERROR: update or delete on table pktableforarray violates foreign key
constraint fktableforarray_ftest1_fkey on table fktableforarray
DETAIL: Key (ptest1)=(2) is still referenced from table fktableforarray.
If I just rip out the first scan, we get the same outcome with a different
error message:
ERROR: removing elements from multidimensional arrays is not supported
CONTEXT: SQL statement UPDATE ONLY public.fktableforarray SET ftest1
= array_remove(ftest1, $1) WHERE $1 OPERATOR(pg_catalog.=) ANY (ftest1)
That has less polish, but I think it's actually more useful. The first
message gives no indication that a multidimensional array foiled your ON
DELETE EACH CASCADE. The second message hints at that cause.
I recommend removing the new block of code in RI_FKey_eachcascade_del() and
letting array_remove() throw the error. If you find a way to throw a nicer
error without an extra scan, by all means submit that to a future CF as an
improvement. I don't think it's important enough to justify cycles at this
late hour of the current CF.
As I pondered this patch again, I came upon formal hazards around
non-default
operator classes. Today, ATAddForeignKeyConstraint() always chooses pfeqop,
ffeqop and ppeqop in the same operator family. If it neglected this, we
would
get wrong behavior when one of the operators is sensitive to a change to
which
another is insensitive. For EACH FK constraints, this patch sets ffeqop =
ARRAY_EQ_OP unconditionally. array_eq() uses the TYPECACHE_EQ_OPR (usually
from the default B-tree operator class). That operator may not exist at
all,
let alone share an operator family with pfeqop. Shall we deal with this by
retrieving TYPECACHE_EQ_OPR in ATAddForeignKeyConstraint() and rejecting the
constraint creation if it does not share an operator family with pfeqop?
The
same hazard affects ri_triggers.c use of array_remove() and array_replace(),
and the same change mitigates it.
Check added. As a consequence of this stricter policy, certain
previously allowed cases are now unacceptable (e.g pk FLOAT and fk
INT[]).
Regression tests have been added.
Ah, good. Not so formal after all.
pg_constraint.confeach needs documentation.
Most of pg_constraint columns, including all the boolean ones, are
documented only in the description column of
http://www.postgresql.org/docs/9.1/static/catalog-pg-constraint.html#AEN85760
it seems that confiseach should not be an exception, since it just
indicates whether the constraint is of a certain kind or not.
Your patch adds two columns to pg_constraint, confiseach and confeach, but it
only mentions confiseach in documentation. Just add a similar minimal mention
of confeach.
***
*** 5736,5741 ATAddForeignKeyConstraint(AlteredTableInfo *tab,
Relation rel,
--- 5759,5807
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg(number of referencing and
referenced columns for foreign key disagree)));
+ /* Enforce each foreign key restrictions */
+ if (fkconstraint-fk_is_each)
+ {
+ /*
+ * ON UPDATE CASCADE action is not supported on EACH
foreign keys
+ */
+ if (fkconstraint-fk_upd_action
Re: [HACKERS] EquivalenceClasses and subqueries and PlaceHolderVars, oh my
On Fri, Mar 16, 2012 at 3:16 PM, Tom Lane t...@sss.pgh.pa.us wrote: So I now propose reverting the earlier two patches (but not their regression test cases of course) and instead hacking MergeAppend plan building as per (2). As a wise man once said, This is tricky stuff. I feel a better that I got stuck on this stuff when you're still trying to feel your way after this many go-arounds. I think I'll be aiming at some less subtle things when I get back to contributing. Things like the getrusage patch I still haven't forgotten about. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
