Re: [HACKERS] pg_upgrade and statistics
On Thu, Mar 15, 2012 at 8:48 PM, Alvaro Herrera > 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 KaiGai 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 wrote: > > Dimitri Fontaine 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 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 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 wrote: > Thom Brown 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 wrote: > On Friday, March 16, 2012 09:30:58 AM Thom Brown wrote: >> On 16 March 2012 08:13, Andres Freund 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 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 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 --- 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)) + HandleAdminActionInter
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 wrote: > > On Friday, March 16, 2012 09:30:58 AM Thom Brown wrote: > >> On 16 March 2012 08:13, Andres Freund 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 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 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; Triggers on commands -PL/pgSQL can be used to define trigger -procedures. A trigger procedure is created with the +PL/pgSQL can be used to define command +trigger procedures. A command trigger procedure is created with the CREATE FUNCTION command, declaring it as a function with no arguments and a return type of command trigger. When a PL/pgSQL 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: - TG_TAG @@ -4002,7 +4001,7 @@ SELECT * FROM sales_summary_bytime; - The command trigger function return's value is not used. + The command trigger function's return value is not used. @@ -4014,23 +4013,20 @@ SELECT * FROM sales_summary_bytime; A PL/pgSQL Command Trigger Procedure - This example trigger just raise a NOTICE message + This example trigger simply raises a NOTICE message each time a supported command is executed. -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 TRIGGER snitch_after AFTER ANY COMMAND EXECUTE PROCEDURE snitch(); diff --git a/doc/src/sgml/ref/create_command_trigger.sgml b/doc/src/sgml/ref/create_command_trigger.sgml index fc12d2e..01c7826 100644 --- a/doc/src/sgml/ref/create_command_trigger.sgml +++ b/doc/src/sgml/ref/cre
Re: [HACKERS] Command Triggers, v16
On 16 March 2012 11:42, Dimitri Fontaine wrote: > Thom Brown 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 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 wrote: > Thom Brown 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 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 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 Eisentraut 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 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 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 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 : > 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 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 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 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 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 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 m
Re: [HACKERS] Command Triggers, v16
On Fri, Mar 16, 2012 at 7:42 AM, Dimitri Fontaine 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 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 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 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 writes: > Tom Lane 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 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 name ALTER DOMAIN name DROP CONSTRAINT [ IF EXISTS ] constraint_name [ RESTRICT | CASCADE ] ALTER DOMAIN name + RENAME CONSTRAINT constraint_name TO new_constraint_name +ALTER DOMAIN name VALIDATE CONSTRAINT constraint_name ALTER DOMAIN name OWNER TO new_owner @@ -103,6 +105,15 @@ ALTER DOMAIN name +RENAME CONSTRAINT + + + This form changes the name of a constraint on a domain. + + + + + VALIDATE CONSTRAINT @@ -182,7 +193,7 @@ ALTER DOMAIN name constraint_name -Name of an existing constraint to drop. +Name of an existing constraint to drop or rename. @@ -226,6 +237,15 @@ ALTER DOMAIN name + new_constraint_name + + +The new name for the constraint. + + + + + new_owner @@ -289,6 +309,13 @@ ALTER DOMAIN zipcode DROP CONSTRAINT zipchk; + To rename a check constraint on a domain: + +ALTER DOMAIN zipcode RENAME CONSTRAINT zipchk TO zip_check; + + + + To move the domain into a different schema: 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 (strcmp(NameStr(con->conname), conname) == 0) { if (OidIsValid(conOid)) -ereport(ERROR, - (errcode(ERRCODE_DUPLICATE_OBJECT), - errmsg("table \"%s\" has multiple constraints named \"%s\"", - get_rel_name(relid), conname))); + { +if (relid) + ereport(ERROR, + (errcode(ERRCODE_DUPLICATE_OBJECT), + errmsg("table \"%s\" has multiple constraints named \"%s\"", + get_rel_name(relid), conname))); +else + ereport(ERROR, + (errcode(ERRCODE_DUPLICATE_OBJECT), + errmsg("domain \"%s\" has multiple constraints named \"%s\"", + format_type_be(typid), conname))); + } conOid = HeapTupleGetOid(tuple); } } @@ -783,10 +806,18 @@ get_constraint_oid(Oid relid, const char *conname, bool missing_ok) /* If no such constraint exists, complain */ if (!OidIsValid(conOid) && !missing_ok) - ereport(ERROR, -(errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("constraint \"%s\" for t
Re: [HACKERS] patch: autocomplete for functions
Peter Eisentraut 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 > 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 http://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 http://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 http://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 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 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 http://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 int
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 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 > > >> 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 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 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 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 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 http://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 wrote: > Dimitri Fontaine writes: >> Tom Lane 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 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 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 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 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 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 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 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 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 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 http://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 http://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(errn
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 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 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 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 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) > > > + { > > > + /* > > > +
Re: [HACKERS] EquivalenceClasses and subqueries and PlaceHolderVars, oh my
On Fri, Mar 16, 2012 at 3:16 PM, Tom Lane 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