Re: [HACKERS] Spin Lock sleep resolution
Hi David, On 02.04.2013 22:58, David Gould wrote: On Tue, 2 Apr 2013 09:01:36 -0700 Jeff Janesjeff.ja...@gmail.com wrote: Sorry. I triple checked that the patch was there, but it seems like if you save a draft with an attachment, when you come back later to finish and send it, the attachment may not be there anymore. The Gmail Offline teams still has a ways to go. Hopefully it is actually there this time. I'll give the patch a try, I have a workload that is impacted by spinlocks fairly heavily sometimes and this might help or at least give me more information. Thanks! Did you ever get around to test this? I repeated these pgbench tests I did earlier: http://www.postgresql.org/message-id/5190e17b.9060...@vmware.com I concluded in that thread that on this platform, the TAS_SPIN macro really needs a non-locked test before the locked one. That fixes the big fall in performance with more than 28 clients. So I repeated that test with four versions: master - no patch spin-delay-ms - Jeff's patch nonlocked-test - master with the non-locked test added to TAS_SPIN spin-delay-ms-nonlocked-test - both patches Jeff's patch seems to somewhat alleviate the huge fall in performance I'm otherwise seeing without the nonlocked-test patch. With the nonlocked-test patch, if you squint you can see a miniscule benefit. I wasn't expecting much of a gain from this, just wanted to verify that it's not making things worse. So looks good to me. - Heikki attachment: clients-sets.png -- 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 for removng unused targets
Hi Alexander, I wrote: From: Tom Lane [mailto:t...@sss.pgh.pa.us] resjunk means that the target is not supposed to be output by the query. Since it's there at all, it's presumably referenced by ORDER BY or GROUP BY or DISTINCT ON, but the meaning of the flag doesn't depend on that. What you would need to do is verify that the target is resjunk and not used in any clause besides ORDER BY. I have not read your patch, but I rather imagine that what you've got now is that the parser checks this and sets the new flag for consumption far downstream. Why not just make the same check in the planner? I've created a patch using this approach. I've rebased the above patch against the latest head. Could you review the patch? If you have no objection, I'd like to mark the patch ready for committer. Thanks, Best regards, Etsuro Fujita unused-targets-20130618.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Spin Lock sleep resolution
On Tue, 18 Jun 2013 10:09:55 +0300 Heikki Linnakangas hlinnakan...@vmware.com wrote: On 02.04.2013 22:58, David Gould wrote: I'll give the patch a try, I have a workload that is impacted by spinlocks fairly heavily sometimes and this might help or at least give me more information. Thanks! Did you ever get around to test this? I repeated these pgbench tests I did earlier: http://www.postgresql.org/message-id/5190e17b.9060...@vmware.com I concluded in that thread that on this platform, the TAS_SPIN macro really needs a non-locked test before the locked one. That fixes the big fall in performance with more than 28 clients. ... I wasn't expecting much of a gain from this, just wanted to verify that it's not making things worse. So looks good to me. Thanks for the followup, and I really like your graph, it looks exactly like what we were hitting. My client ended up configuring around it and adding more hosts so the urgency to run more tests sort of declined, although I think we still hit it from time to time. If you would like to point me at or send me the latest flavor of the patch it may be timely for me to test again. Especially if this is a more or less finished version, we are about to roll out a new build to all these hosts and I'd be happy to try to incorporate this patch and get some production experience with it on 80 core hosts. -dg -- David Gould 510 282 0869 da...@sonic.net If simplicity worked, the world would be overrun with insects. -- 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] dynamic background workers
Hi,
On Sat, Jun 15, 2013 at 6:00 AM, Robert Haas robertmh...@gmail.com wrote:
The first patch, max-worker-processes-v1.patch, adds a new GUC
max_worker_processes, which defaults to 8. This fixes the problem
discussed here:
http://www.postgresql.org/message-id/CA+TgmobguVO+qHnHvxBA2TFkDhw67Y=4bp405fvabec_eto...@mail.gmail.com
Apart from fixing that problem, it's a pretty boring patch.
I just had a look at the first patch which is pretty simple before
looking in details at the 2nd patch.
Here are some minor comments:
1) Correction of postgresql.conf.sample
Putting the new parameter in the section resource usage is adapted,
however why not adding a new sub-section of this type with some
comments like below?
# - Background workers -
#max_worker_processes = 8 # Maximum number of background worker subprocesses
# (change requires restart)
2) Perhaps it would be better to specify in the docs that if the
number of bgworker that are tried to be started by server is higher
than max_worker_processes, startup of the extra bgworkers will fail
but server will continue running as if nothing happened. This is
something users should be made aware of.
3) In InitProcGlobal:proc.c, wouldn't it be more consistent to do that
when assigning new slots in PGPROC:
else if (i MaxConnections + autovacuum_max_workers + max_worker_processes + 1)
{
/* PGPROC for bgworker, add to bgworkerFreeProcs list */
procs[i].links.next = (SHM_QUEUE *) ProcGlobal-bgworkerFreeProcs;
ProcGlobal-bgworkerFreeProcs = procs[i];
}
instead of that?
else if (i MaxBackends)
{
/* PGPROC for bgworker, add to bgworkerFreeProcs list */
procs[i].links.next = (SHM_QUEUE *) ProcGlobal-bgworkerFreeProcs;
ProcGlobal-bgworkerFreeProcs = procs[i];
}
I have also done many tests with worker_spi and some home-made
bgworkers and the patch is working as expected, the extra bgworkers
are not started once the maximum number set it reached.
I'll try to look at the other patch soon, but I think that the real
discussion on the topic is just beginning... Btw, IMHO, this first
patch can safely be committed as we would have a nice base for future
discussions/reviews.
Regards,
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] extensible external toast tuple support
On Fri, Jun 14, 2013 at 4:06 PM, Andres Freund and...@2ndquadrant.comwrote: Here's the updated version. It shouldn't contain any obvious WIP pieces anymore, although I think it needs some more documentation. I am just not sure where to add it yet, postgres.h seems like a bad place :/ I have a few comments and questions after reviewing this patch. - heap_tuple_fetch_attr doesn't need to be updated to reflect ONDISK macro? - I'm not sure if plural for datum is good to use. Datum values? - -1 from me to use enum for tag types, as I don't think it needs to be. This looks more like other kind macros such as relkind, but I know there's pros/cons - don't we need cast for tag value comparison in VARTAG_SIZE macro, since tag is unit8 and enum is signed int? - Is there better way to represent ONDISK size, instead of magic number 18? I'd suggest constructing it with sizeof(varatt_external). Other than that, the patch applies fine and make check runs, though I don't think the new code path is exercised well, as no one is creating INDIRECT datum yet. Also, I wonder how we could add more compression in datum, as well as how we are going to add more compression options in database. I'd love to see pluggability here, as surely the core cannot support dozens of different compression algorithms, but because the data on disk is critical and we cannot do anything like user defined functions. The algorithms should be optional builtin so that once the system is initialized the the plugin should not go away. Anyway pluggable compression is off-topic here. Thanks, -- Hitoshi Harada
Re: [HACKERS] Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls
On Sat, Jun 15, 2013 at 1:30 PM, Jeff Davis pg...@j-davis.com wrote: On Sun, 2013-03-24 at 20:15 -0400, Nicholas White wrote: I've redone the leadlag function changes to use datumCopy as you suggested. However, I've had to remove the NOT_USED #ifdef around datumFree so I can use it to avoid building up large numbers of copies of Datums in the memory context while a query is executing. I've attached the revised patch... Comments: WinGetResultDatumCopy() calls datumCopy, which will just copy in the current memory context. I think you want it in the per-partition memory context, otherwise the last value in each partition will stick around until the query is done (so many partitions could be a problem). That should be easy enough to do by switching to the winobj-winstate-partcontext memory context before calling datumCopy, and then switching back. For that matter, why store the datum again at all? You can just store the offset of the last non-NULL value in that partition, and then fetch it using WinGetFuncArgInPartition(), right? We really want to avoid any per-tuple allocations. I believe WinGetFuncArgInPartition is a bit slow if the offset is far from the current row. So it might make sense to store the last-seen value, but I'm not sure if we need to copy datum every time. I haven't looked into the new patch. Thanks, -- Hitoshi Harada
Re: [HACKERS] Batch API for After Triggers
On 17 June 2013 20:53, Kevin Grittner kgri...@ymail.com wrote: Simon Riggs si...@2ndquadrant.com wrote: On 9 June 2013 12:58, Craig Ringer cr...@2ndquadrant.com wrote: We don't currently have OLD and NEW relations so we're free to define how this works pretty freely. I think the best way, if we did do this, would be to have a number of different relations defined: OLD NEW INSERTED DELETED all of which would be defined same as main table and also one called UPDATED which would have two row vars called OLD and NEW so you would access it like e.g. IF UPDATED.OLD.id = 7 Well, there is the SQL standard, which has a couple paragraphs on the topic which we might want to heed. Yes, I already did in my proposal above. OLD and NEW are all we need to fulfill the standard. For a delete there is just an old table; for an insert just a new one. For an update you have both, with the same cardinality. The rows in the old and new tables have a correspondence, but that is only visible to FOR EACH ROW triggers. Yes, those are the relevant parts. SQL:2008 4.38 is the paragraphs that describe this (for later reference). What the standard doesn't cover is recursive calls, that might generate new events of different kinds. So an UPDATE statement might have caused DELETEs etc.. So we'd need a way to get access to DELETED rows even when the OLD relation covers only the UPDATED rows. For row level triggers we support macros like TRIGGER_FIRED_BY_INSERT. Having an INSERT relation is just the logical equivalent for statement level triggers. For something like RI, why would you need to establish correspondence? A row with the referenced key either exists after the statement completes, or it doesn't -- why would we care whether it is an updated version of the same row? I wasn't doing this for RI specifically, I was looking at what we'd need to provide a full facilitiy. It's not very easy to see how we can support RI via statement level triggers. By definiton, statement level triggers happen after all row level triggers have fired. So implementing row level RI by using statement level triggers that follow the standard isn't possible. The main things we'd need to cope with would be recursive trigger calls, for example DELETE cascades. The firing of the triggers generates more trigger events which delete more rows etc.. If we want to implement RI triggers using some form of set processing we would need to do that in the middle of handling the after row events, i.e. execute a set, then re-check for a new set of events and execute them. Directly using the statement-level standard triggers isn't the way, I conclude after some detailed thinking. But there could be some intermediate form that makes sense. Syntax for how to refer to the these is defined by the standard. As usual, I don't object to adding capabilities as long as the standard syntax is also supported with standard semantics. Agreed. -- 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] Batch API for After Triggers
On 17 June 2013 23:30, Craig Ringer cr...@2ndquadrant.com wrote: INSERTED and UPDATED could just be views... Yes, that would be my suggestion. -- 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] Spin Lock sleep resolution
On 18.06.2013 10:52, David Gould wrote: On Tue, 18 Jun 2013 10:09:55 +0300 Heikki Linnakangashlinnakan...@vmware.com wrote: I repeated these pgbench tests I did earlier: http://www.postgresql.org/message-id/5190e17b.9060...@vmware.com I concluded in that thread that on this platform, the TAS_SPIN macro really needs a non-locked test before the locked one. That fixes the big fall in performance with more than 28 clients. ... I wasn't expecting much of a gain from this, just wanted to verify that it's not making things worse. So looks good to me. Thanks for the followup, and I really like your graph, it looks exactly like what we were hitting. My client ended up configuring around it and adding more hosts so the urgency to run more tests sort of declined, although I think we still hit it from time to time. Oh, interesting. What kind of hardware are you running on? To be honest, I'm not sure what my test hardware is, it's managed by another team across the world, but /proc/cpuinfo says: model name : Intel(R) Xeon(R) CPU E5-4640 0 @ 2.40GHz And it's running in a virtual machine on VMware; that might also be a factor. It would be good to test the TAS_SPIN nonlocked patch on a variety of systems. The comments in s_lock.h say that on Opteron, the non-locked test is a huge loss. In particular, would be good to re-test that on a modern AMD system. If you would like to point me at or send me the latest flavor of the patch it may be timely for me to test again. Especially if this is a more or less finished version, we are about to roll out a new build to all these hosts and I'd be happy to try to incorporate this patch and get some production experience with it on 80 core hosts. Jeff's patch is here: http://www.postgresql.org/message-id/CAMkU=1xtp+3uwl6dg10rdrtma28zy-9ujroxnust_r3zodp...@mail.gmail.com. My non-locked TAS_SPIN patch is the one-liner here: http://www.postgresql.org/message-id/519a938a.1070...@vmware.com Thanks! - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] A minor correction in comment in heaptuple.c
Hi, Should it be: return true if attnum is out of range according to the tupdesc instead of return NULL if attnum is out of range according to the tupdesc at src/backend/access/common/heaptuple.c: 1345 /* * return true if attnum is out of range according to the tupdesc */ if (attnum tupleDesc-natts) return true; Attached patch fixes this. -- Amit Langote correct-a-comment-in-heaptupledotc.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] extensible external toast tuple support
On 2013-06-18 00:56:17 -0700, Hitoshi Harada wrote: On Fri, Jun 14, 2013 at 4:06 PM, Andres Freund and...@2ndquadrant.comwrote: Here's the updated version. It shouldn't contain any obvious WIP pieces anymore, although I think it needs some more documentation. I am just not sure where to add it yet, postgres.h seems like a bad place :/ I have a few comments and questions after reviewing this patch. Cool! - heap_tuple_fetch_attr doesn't need to be updated to reflect ONDISK macro? It calls toast_fetch_datum() for any kind of external datum, so it should be fine since ONDISK is handled in there. - I'm not sure if plural for datum is good to use. Datum values? Not sure at all either. Supposedly data is the plural for datum, but for the capitalized version Datums is used in other places, so I think it might be fine. - -1 from me to use enum for tag types, as I don't think it needs to be. This looks more like other kind macros such as relkind, but I know there's pros/cons Well, relkind cannot easily be a enum because it needs to be stored in a char field. I like using enums because it gives you the chance of using switch()es at some point and getting warned about missed cases. Why do you dislike it? - don't we need cast for tag value comparison in VARTAG_SIZE macro, since tag is unit8 and enum is signed int? I think it should be fine (and the compiler doesn't warn) due to the integer promotion rules. - Is there better way to represent ONDISK size, instead of magic number 18? I'd suggest constructing it with sizeof(varatt_external). I explicitly did not want to do that, since the numbers really don't have anything to do with the struct size anymore. Otherwise the next person reading that part will be confused because there's a 8byte struct with the enum value 1. Or somebody will try adding another type of external tuple that also needs 18 bytes by copying the sizeof(). Which will fail in ugly, non-obvious ways. Other than that, the patch applies fine and make check runs, though I don't think the new code path is exercised well, as no one is creating INDIRECT datum yet. Yea, I only use the API in the changeset extraction patch. That actually worries me to. But I am not sure where to introduce usage of it in core without making the patchset significantly larger. I was thinking of adding an option to heap_form_tuple/heap_fill_tuple to allow it to reference _4B Datums instead of copying them, but that would require quite some adjustments on the callsites. Also, I wonder how we could add more compression in datum, as well as how we are going to add more compression options in database. I'd love to see pluggability here, as surely the core cannot support dozens of different compression algorithms, but because the data on disk is critical and we cannot do anything like user defined functions. The algorithms should be optional builtin so that once the system is initialized the the plugin should not go away. Anyway pluggable compression is off-topic here. Separate patchset by now, yep ;). Thanks! Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A minor correction in comment in heaptuple.c
Hi, On 2013-06-18 17:56:34 +0900, Amit Langote wrote: Should it be: return true if attnum is out of range according to the tupdesc instead of return NULL if attnum is out of range according to the tupdesc at src/backend/access/common/heaptuple.c: 1345 /* * return true if attnum is out of range according to the tupdesc */ if (attnum tupleDesc-natts) return true; Well, true actually tells us that the attribute is null, since that's the purpose of the function: /* * slot_attisnull * Detect whether an attribute of the slot is null, without * actually fetching it. */ I think the comment is more meaningfull before the change since it tells us how nonexisting columns are interpreted. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Tue, Jun 18, 2013 at 6:01 PM, Andres Freund and...@2ndquadrant.com wrote: Hi, On 2013-06-18 17:56:34 +0900, Amit Langote wrote: Should it be: return true if attnum is out of range according to the tupdesc instead of return NULL if attnum is out of range according to the tupdesc at src/backend/access/common/heaptuple.c: 1345 /* * return true if attnum is out of range according to the tupdesc */ if (attnum tupleDesc-natts) return true; Well, true actually tells us that the attribute is null, since that's the purpose of the function: /* * slot_attisnull * Detect whether an attribute of the slot is null, without * actually fetching it. */ I think the comment is more meaningfull before the change since it tells us how nonexisting columns are interpreted. Okay, that makes sense. -- Amit Langote -- 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] A minor correction in comment in heaptuple.c
On Tue, 18 Jun 2013 11:01:28 +0200 Andres Freund and...@2ndquadrant.com wrote: /* * return true if attnum is out of range according to the tupdesc */ if (attnum tupleDesc-natts) return true; I think the comment is more meaningfull before the change since it tells us how nonexisting columns are interpreted. I think that the comment is bad either way. Comments should explain the code, not repeat it. The above is not far removed from... return 5; /* return the number 5 */ How about check if attnum is out of range according to the tupdesc instead? -- D'Arcy J.M. Cain da...@druid.net | Democracy is three wolves http://www.druid.net/darcy/| and a sheep voting on +1 416 788 2246 (DoD#0082)(eNTP) | what's for dinner. IM: da...@vex.net, VOIP: sip:da...@vex.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] Change authentication error message (patch)
On 06/16/2013 06:02 PM, Joshua D. Drake wrote: Instead of pushing extra info to the logs I decided that we could without giving away extra details per policy. I wrote the error message in a way that tells the most obvious problems, without admitting to any of them. Please see attached: +1 for solving this with a bit of word-smithing. However, the proposed wording doesn't sound like a full sentence to my ears, because a password or username cannot fail per-se. How about: password authentication failed or account expired for user \%s\ It's a bit longer, but sounds more like a full sentence, no? Regards Markus Wanner -- 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] Support for REINDEX CONCURRENTLY
Hi,
On 2013-06-18 10:53:25 +0900, Michael Paquier wrote:
diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c
index c381f11..3a6342c 100644
--- a/contrib/pg_upgrade/info.c
+++ b/contrib/pg_upgrade/info.c
@@ -321,12 +321,17 @@ get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo)
INSERT INTO
info_rels
SELECT reltoastrelid
FROM info_rels i
JOIN pg_catalog.pg_class c
-ON
i.reloid = c.oid));
+ON
i.reloid = c.oid
+AND
c.reltoastrelid != %u, InvalidOid));
PQclear(executeQueryOrDie(conn,
INSERT INTO
info_rels
- SELECT reltoastidxid
- FROM info_rels i
JOIN pg_catalog.pg_class c
-ON
i.reloid = c.oid));
+ SELECT indexrelid
+ FROM pg_index
+ WHERE indrelid IN
(SELECT reltoastrelid
+ FROM
pg_class
+ WHERE oid
= %u
+AND
reltoastrelid != %u),
+ FirstNormalObjectId,
InvalidOid));
What's the idea behind the = here?
I think we should ignore the invalid indexes in that SELECT?
@@ -1392,19 +1390,62 @@ swap_relation_files(Oid r1, Oid r2, bool
target_is_pg_class,
}
/*
- * If we're swapping two toast tables by content, do the same for their
- * indexes.
+ * If we're swapping two toast tables by content, do the same for all of
+ * their indexes. The swap can actually be safely done only if the
+ * relations have indexes.
*/
if (swap_toast_by_content
- relform1-reltoastidxid relform2-reltoastidxid)
- swap_relation_files(relform1-reltoastidxid,
- relform2-reltoastidxid,
- target_is_pg_class,
- swap_toast_by_content,
- is_internal,
- InvalidTransactionId,
- InvalidMultiXactId,
- mapped_tables);
+ relform1-reltoastrelid
+ relform2-reltoastrelid)
+ {
+ Relation toastRel1, toastRel2;
+
+ /* Open relations */
+ toastRel1 = heap_open(relform1-reltoastrelid,
AccessExclusiveLock);
+ toastRel2 = heap_open(relform2-reltoastrelid,
AccessExclusiveLock);
+
+ /* Obtain index list */
+ RelationGetIndexList(toastRel1);
+ RelationGetIndexList(toastRel2);
+
+ /* Check if the swap is possible for all the toast indexes */
+ if (list_length(toastRel1-rd_indexlist) == 1
+ list_length(toastRel2-rd_indexlist) == 1)
+ {
+ ListCell *lc1, *lc2;
+
+ /* Now swap each couple */
+ lc2 = list_head(toastRel2-rd_indexlist);
+ foreach(lc1, toastRel1-rd_indexlist)
+ {
+ Oid indexOid1 = lfirst_oid(lc1);
+ Oid indexOid2 = lfirst_oid(lc2);
+ swap_relation_files(indexOid1,
+
indexOid2,
+
target_is_pg_class,
+
swap_toast_by_content,
+
is_internal,
+
InvalidTransactionId,
+
InvalidMultiXactId,
+
mapped_tables);
+ lc2 = lnext(lc2);
+ }
Why are you iterating over the indexlists after checking they are both
of length == 1?
Re: [HACKERS] A minor correction in comment in heaptuple.c
On 2013-06-18 05:21:15 -0400, D'Arcy J.M. Cain wrote: On Tue, 18 Jun 2013 11:01:28 +0200 Andres Freund and...@2ndquadrant.com wrote: /* * return true if attnum is out of range according to the tupdesc */ if (attnum tupleDesc-natts) return true; I think the comment is more meaningfull before the change since it tells us how nonexisting columns are interpreted. I think that the comment is bad either way. Comments should explain the code, not repeat it. The above is not far removed from... return 5; /* return the number 5 */ How about check if attnum is out of range according to the tupdesc instead? I can't follow. Minus the word 'NULL' - which carries meaning - your suggested comment pretty much is the same as the existing comment except that you use 'check' instead of 'return'. Original: /* * return NULL if attnum is out of range according to the tupdesc */ if (attnum tupleDesc-natts) return true; Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Hi,
review below.
2013-06-13 14:35 keltezéssel, Amit Kapila írta:
On Friday, June 07, 2013 9:45 AM Amit Kapila wrote:
On Thursday, June 06, 2013 10:22 PM Robert Haas wrote:
On Wed, Jun 5, 2013 at 7:24 AM, Amit Kapila amit.kap...@huawei.com
wrote:
On Monday, May 27, 2013 4:17 PM Amit Kapila wrote:
On Wednesday, April 03, 2013 11:55 AM Amit Kapila wote:
On Tuesday, April 02, 2013 9:49 PM Peter Eisentraut wrote:
There are 2 options to proceed for this patch for 9.4
1. Upload the SET PERSISTENT syntax patch for coming CF by fixing
existing
review comments
2. Implement new syntax ALTER SYSTEM as proposed in below mail
Could you suggest me what could be best way to proceed for this
patch?
I'm still in favor of some syntax involving ALTER, because it's still
true that this behaves more like the existing GUC-setting commands
that use ALTER (which change configuration for future sessions)
rather
the ones that use SET (which change the current settings for some
period of time).
I will change the patch as per below syntax if there are no objections:
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value'};
Modified patch contains:
1. Syntax implemented is
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value' |
DEFAULT};
If user specifies DEFAULT, it will remove entry from auto conf file.
2. File name to store settings set by ALTER SYSTEM command is still
persistent.auto.conf
3. Added a new page for Alter System command in docs. In compatability
section, I had mentioned that
this statement is an PostgresQL extension. I had tried to search in
SQL-92 spec but couldn't find such command.
4. During test of this patch, I had observed one problem for PGC_BACKEND
parameters like log_connections.
If I change these parameters directly in postgresql.conf and then do
pg_reload_conf() and reconnect, it will
still show the old value. This is observed only on WINDOWS. I will
investigate this problem and update you.
Due to this problem, I had removed few test cases.
I can't reproduce this error under Linux (Fedora 19/x86_64).
The bug might be somewhere else and not caused by your patch
if manually adding/removing log_connections = on from postgresql.conf
exhibits the same behaviour under Windows. (If I read it correctly,
you tested it exactly this way.) Does the current GIT version behave
the same way?
Kindly let me know your suggestions.
With Regards,
Amit Kapila.
* Is the patch in a patch format which has context?
Yes.
* Does it apply cleanly to the current git master?
Yes.
* Does it include reasonable tests, necessary doc patches, etc?
Yes.
Read what the patch is supposed to do, and consider:
* Does the patch actually implement that?
Yes.
* Do we want that?
Yes.
* Do we already have it?
No.
* Does it follow SQL spec, or the community-agreed behavior?
No such spec, follows the agreed behaviour.
* Does it include pg_dump support (if applicable)?
N/A
* Are there dangers?
No.
* Have all the bases been covered?
According to the above note under Windows, not yet.
The name persistent.auto.conf is not yet set in stone.
postgresql.auto.conf might be better.
Apply the patch, compile it and test:
* Does the feature work as advertised?
Yes, with the noted exception above for log_connections.
* Are there corner cases the author has failed to consider?
I can't see any. It is through
* Are there any assertion failures or crashes?
No.
* Does the patch slow down simple tests?
No.
* If it claims to improve performance, does it?
N/A
* Does it slow down other things?
No.
* Does it follow the project coding guidelines
http://developer.postgresql.org/pgdocs/postgres/source.html?
Mostly, yes. Some nits, though. At some places, you do:
- ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head, tail);
+ ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head, tail,NULL);
without a space between the last comma and the NULL keyword.
Also, in the comment above ParseConfigFile() there are unnecessary
white space changes and spaces at the end of the line.
* Are there portability issues?
No.
* Will it work on Windows/BSD etc?
It should. It doesn't use any platform-dependent code.
* Are the comments sufficient and accurate?
Yes.
* Does it do what it says, correctly?
Yes.
* Does it produce compiler warnings?
No.
* Can you make it crash?
No.
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
2013-06-14 05:12 keltezéssel, Amit Kapila írta:
On Friday, June 14, 2013 3:17 AM Josh Berkus wrote:
On 06/13/2013 05:35 AM, Amit Kapila wrote:
On Friday, June 07, 2013 9:45 AM Amit Kapila wrote:
On Thursday, June 06, 2013 10:22 PM Robert Haas wrote:
On Wed, Jun 5, 2013 at 7:24 AM, Amit Kapila
amit.kap...@huawei.com
wrote:
On Monday, May 27, 2013 4:17 PM Amit Kapila wrote:
On Wednesday, April 03, 2013 11:55 AM Amit Kapila wote:
On Tuesday, April 02, 2013 9:49 PM Peter Eisentraut wrote:
There are 2 options to proceed for this patch for 9.4
1. Upload the SET PERSISTENT syntax patch for coming CF by fixing
existing
review comments
2. Implement new syntax ALTER SYSTEM as proposed in below mail
Could you suggest me what could be best way to proceed for this
patch?
I'm still in favor of some syntax involving ALTER, because it's
still
true that this behaves more like the existing GUC-setting commands
that use ALTER (which change configuration for future sessions)
rather
the ones that use SET (which change the current settings for some
period of time).
I will change the patch as per below syntax if there are no
objections:
ALTER SYSTEM SET configuration_parameter {TO | =} {value, |
'value'};
Modified patch contains:
1. Syntax implemented is
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value' |
DEFAULT};
If user specifies DEFAULT, it will remove entry from auto conf file.
2. File name to store settings set by ALTER SYSTEM command is still
persistent.auto.conf
Why? Shouldn't it just be auto.conf? Or system.auto.conf?
I had kept same name as it was decided for 9.3, but now as command has
changed so it makes more
sense to change name as well. I think it can be one of what you suggested or
postgresql.auto.conf.
I prefer auto.conf, personally.
Thanks. if no body has any other suggestion I will change it.
I think one of system.auto.conf or postgresql.auto.conf is more appropriate because it either resembles command or existing configuration settings
file.
I like postgresql.auto.conf better.
With Regards,
Amit Kapila.
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add regression tests for SET xxx
On 18 June 2013 02:33, Robins Tharakan thara...@gmail.com wrote: Thanks ! PFA the updated patch. Also remove a trailing whitespace at the end of SQL script. -- Robins Tharakan On 17 June 2013 17:29, Szymon Guz mabew...@gmail.com wrote: On 26 May 2013 19:56, Robins Tharakan thara...@gmail.com wrote: Hi, Please find attached a patch to take code-coverage of SET (SESSION / SEED / TRANSACTION / DATESTYLE / TIME ZONE) ( src/backend/commands/variable.c) from 65% to 82%. Any and all feedback is welcome. -- Robins Tharakan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Hi, the patch applies cleanly on code from trunk, however there are failing tests, diff attached. regards Szymon Hi, I've checked the patch. Applies cleanly. Tests pass this time :) Could you add me as a reviewer to commitfest website, set this patch a reviewed and add this email to the patch history? I cannot login to the commitfest app, there is some bug with that. thanks, Szymon Guz
Re: [HACKERS] How do we track backpatches?
Le mardi 18 juin 2013 04:48:02, Peter Eisentraut a écrit : On Mon, 2013-06-17 at 17:11 -0700, Josh Berkus wrote: Contributors, While going through this mailing list looking for missing 9.4 patches, I realized that we don't track backpatches (that is, fixes to prior versions) at all anywhere. Where backpatches are submitted by committers this isn't an issue, but we had a couple major ones (like the autovacuum fix) which were submitted by general contributors. The same goes for beta fixes. Should we add a prior version category to the CF to make sure these don't get dropped? Or do something else? A separate commit fest for tracking proposed backpatches might be useful. Backpatches are bugs fix, isnt it ? I will like to have a mail based bug tracker like debbugs. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation signature.asc Description: This is a digitally signed message part.
Re: [HACKERS] How do we track backpatches?
On Tue, Jun 18, 2013 at 4:48 AM, Peter Eisentraut pete...@gmx.net wrote: On Mon, 2013-06-17 at 17:11 -0700, Josh Berkus wrote: Contributors, While going through this mailing list looking for missing 9.4 patches, I realized that we don't track backpatches (that is, fixes to prior versions) at all anywhere. Where backpatches are submitted by committers this isn't an issue, but we had a couple major ones (like the autovacuum fix) which were submitted by general contributors. The same goes for beta fixes. Should we add a prior version category to the CF to make sure these don't get dropped? Or do something else? A separate commit fest for tracking proposed backpatches might be useful. The CF app was and is specifically for dealing with CFs. Having it deal with backpatches makes it, well, a bugtracker. It's not meant to be that. If we want a bugtracker, then it has very different requirements. Having an always-open CF would defeat the workflow. But since those patches are typically going into HEAD as well, why not just a commitfest *topic* for it, on whatever commitfest happens to be the open one? Then it'll get processed within the existing workflow. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Review: UNNEST (and other functions) WITH ORDINALITY
On 17 June 2013 06:33, David Fetter da...@fetter.org wrote: Next revision of the patch, now with more stability. Thanks, Andrew! Rebased vs. git master. Here's my review of the WITH ORDINALITY patch. Overall I think that the patch is in good shape, and I think that this will be a very useful new feature, so I'm keen to see this committed. All the basic stuff is OK --- the patch applies cleanly, compiles with no warnings, and has appropriate docs and new regression tests which pass. I also tested it fairly thoroughly myself, and I couldn't break it. Everything worked as I expected, and it works nicely with LATERAL. I have a few minor comments that should be considered before passing it on to a committer: 1). In func.sgml, the new doc example unnest WITH ORDINALITY is mislablled, since it it's not actually an example of unnest(). Also it doesn't really belong in that code example block, which is about generate_subscripts(). I think that there should probably be a new sub-section starting at that point for WITH ORDINALITY. Perhaps it should start with a brief paragraph explaining how WITH ORDINALITY can be applied to functions in the FROM clause of a query. [Actually it appears that WITH ORDINALITY works with non-SRF's too, but that's less useful, so I think that the SRF section is probably still the right place to document this] It might also be worth mentioning here that currently WITH ORDINALITY is not supported for functions that return records. In the code example itself, the prompt should be trimmed down to match the previous examples. 2). In the SELECT docs, where function_name is documented, I would be tempted to start a new paragraph for the sentence starting If the function has been defined as returning the record data type..., since that's really a separate syntax. I think that should also make mention of the fact that WITH ORDINALITY is not currently supported in that case. 3). I think it would be good to have a more meaningful default name for the new ordinality column, rather than ?column?. In many cases the user might then choose to not bother giving it an alias. It could simply be called ordinality by default, since that's non-reserved. 4). In gram.y, WITH_ORDINALITY should not be listed as an ordinary keyword, but instead should be listed as a token below that (just before WITH_TIME). 5). In plannodes.h, FunctionScan's new field should probably have a comment. 6). In parsenodes.h, the field added to RangeTblEntry is only valid for function RTEs, so it should be moved to that group of fields and renamed appropriately (unless you're expecting to extend it to other RTE kinds in the future?). Logically then, the new check for ordinality in inline_set_returning_function() should be moved so that it is after the check that the RTE actually a function RTE, and in addRangeTableEntryForFunction() the RTE's ordinality field should be set at the start along with the other function-related fields. 7). In execnodes.h, the new fields added to FunctionScanState should be documented in the comment block above. Overall, as I said, I really like this feature, and I think it's not far from being commitable. Regards, Dean -- 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] How do we track backpatches?
On 2013-06-18 12:32:42 +0200, Magnus Hagander wrote: On Tue, Jun 18, 2013 at 4:48 AM, Peter Eisentraut pete...@gmx.net wrote: On Mon, 2013-06-17 at 17:11 -0700, Josh Berkus wrote: Contributors, While going through this mailing list looking for missing 9.4 patches, I realized that we don't track backpatches (that is, fixes to prior versions) at all anywhere. Where backpatches are submitted by committers this isn't an issue, but we had a couple major ones (like the autovacuum fix) which were submitted by general contributors. The same goes for beta fixes. Should we add a prior version category to the CF to make sure these don't get dropped? Or do something else? A separate commit fest for tracking proposed backpatches might be useful. The CF app was and is specifically for dealing with CFs. Having it deal with backpatches makes it, well, a bugtracker. It's not meant to be that. If we want a bugtracker, then it has very different requirements. Having an always-open CF would defeat the workflow. But since those patches are typically going into HEAD as well, why not just a commitfest *topic* for it, on whatever commitfest happens to be the open one? Then it'll get processed within the existing workflow. The schedules for a CF and a minor release don't really line up though, so I am not sure if that ends up being much better than not tracking them there... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add regression tests for SET xxx
On Tue, Jun 18, 2013 at 7:01 PM, Szymon Guz mabew...@gmail.com wrote: Could you add me as a reviewer to commitfest website, set this patch a reviewed and add this email to the patch history? I cannot login to the commitfest app, there is some bug with that. You should be able to do it yourself by creating a community account in postgresql.org. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add regression tests for SET xxx
On 18 June 2013 13:10, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Jun 18, 2013 at 7:01 PM, Szymon Guz mabew...@gmail.com wrote: Could you add me as a reviewer to commitfest website, set this patch a reviewed and add this email to the patch history? I cannot login to the commitfest app, there is some bug with that. You should be able to do it yourself by creating a community account in postgresql.org. -- Michael Yea, I know. Unfortunately there is a bug and currently you cannot login to commitfest using a new account, or old, if you changed password like I did. I've got a bug confirmation from Magnus on pgsql-www list. thanks, Szymon
[HACKERS] Memory leak in PL/pgSQL function which CREATE/SELECT/DROP a temporary table
Hello, I've encountered a memory leak problem when I use a PL/pgsql function which creates and drops a temporary table. I couldn't find any similar problem in the mailing list. I'd like to ask you whether this is a PostgreSQL's bug. Maybe I should post this to pgsql-bugs or pgsql-general, but the discussion is likely to involve the internal behavior of PostgreSQL, so let me post here. The steps to reproduce the problem is as follows. Please find attached two files to use for this. $ psql -d postgres -f myfunc.sql $ ecpg myfunc.pgc $ cc -Ipgsql_inst_dir/include myfunc.c -o myfunc -Lpg_inst_dir/lib -lecpg $ ./myfunc As the program myfunc runs longer, the values of VSZ and RSS get bigger, even after 50,000 transactions. The cause of the memory increase appears to be CacheMemoryContext. When I attached to postgres with gdb and ran call MemoryContextStats(TopMemoryContext) several times, the size of CacheMemoryContext kept increasing. By the way, when I replace SELECT COUNT(*) INTO cnt FROM mytable in the PL/pgSQL function with INSERT INTO mytable VALUES(1), the memory stops increasing. So, the memory leak seems to occur when SELECT is used. I know the solution -- add IF NOT EXISTS to the CREATE TEMPORARY TABLE. That prevents memory increase. But why? What's wrong with my program? I'd like to know: Q1: Is this a bug of PostgreSQL? Q2: If yes, is it planned to be included in the upcoming minor release? Q3: If this is not a bug and a reasonable behavior, is it described anywhere? Regards MauMau myfunc.pgc Description: Binary data myfunc.sql Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Spin Lock sleep resolution
On Tue, 18 Jun 2013 11:41:06 +0300 Heikki Linnakangas hlinnakan...@vmware.com wrote: Oh, interesting. What kind of hardware are you running on? To be honest, I'm not sure what my test hardware is, it's managed by another team across the world, but /proc/cpuinfo says: model name: Intel(R) Xeon(R) CPU E5-4640 0 @ 2.40GHz It claims to have 80 of these: model name : Intel(R) Xeon(R) CPU E7-L8867 @2.13GHz Postgres is on ramfs on these with unlogged tables. And it's running in a virtual machine on VMware; that might also be a factor. I'm not a fan of virtualization. It makes performance even harder to reason about. It would be good to test the TAS_SPIN nonlocked patch on a variety of systems. The comments in s_lock.h say that on Opteron, the non-locked test is a huge loss. In particular, would be good to re-test that on a modern AMD system. I'll see what I can do. However I don't have acces to any large modern AMD systems. -dg -- David Gould 510 282 0869 da...@sonic.net If simplicity worked, the world would be overrun with insects. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
On Tuesday, June 18, 2013 3:26 PM Boszormenyi Zoltan wrote:
Hi,
review below.
Thanks for the review.
There are 2 options to proceed for this patch for 9.4
1. Upload the SET PERSISTENT syntax patch for coming CF by fixing
existing
review comments
2. Implement new syntax ALTER SYSTEM as proposed in below mail
Could you suggest me what could be best way to proceed for this
patch?
I'm still in favor of some syntax involving ALTER, because it's still
true that this behaves more like the existing GUC-setting commands
that use ALTER (which change configuration for future sessions)
rather
the ones that use SET (which change the current settings for some
period of time).
I will change the patch as per below syntax if there are no objections:
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value'};
Modified patch contains:
1. Syntax implemented is
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value' |
DEFAULT};
If user specifies DEFAULT, it will remove entry from auto conf file.
2. File name to store settings set by ALTER SYSTEM command is still
persistent.auto.conf
3. Added a new page for Alter System command in docs. In compatability
section, I had mentioned that
this statement is an PostgresQL extension. I had tried to search in
SQL-92 spec but couldn't find such command.
4. During test of this patch, I had observed one problem for PGC_BACKEND
parameters like log_connections.
If I change these parameters directly in postgresql.conf and then do
pg_reload_conf() and reconnect, it will
still show the old value. This is observed only on WINDOWS. I will
investigate this problem and update you.
Due to this problem, I had removed few test cases.
I can't reproduce this error under Linux (Fedora 19/x86_64).
The bug might be somewhere else and not caused by your patch
if manually adding/removing log_connections = on from postgresql.conf
exhibits the same behaviour under Windows. (If I read it correctly,
you tested it exactly this way.) Does the current GIT version behave
the same way?
Yes, the current Git has problem which I had reported in below mail:
http://www.postgresql.org/message-id/009801ce68f7$3a746340$af5d29c0$@kapila@
huawei.com
This problem is not related to this patch, it occurs only on WINDOWS or
under EXEC_BACKEND flag.
I think we can discuss this problem in above mail thread.
* Have all the bases been covered?
According to the above note under Windows, not yet.
The name persistent.auto.conf is not yet set in stone.
postgresql.auto.conf might be better.
I think, the decision of name, we can leave to committer with below
possibilities,
as it is very difficult to build consensus on any particular name.
Auto.conf
System.auto.conf
Postgresql.auto.conf
Persistent.auto.conf
Apply the patch, compile it and test:
* Does it follow the project coding guidelines?
Mostly, yes. Some nits, though. At some places, you do:
- ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head, tail);
+ ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head,
tail,NULL);
I think you mean ParseConfigFp()?
without a space between the last comma and the NULL keyword.
Also, in the comment above ParseConfigFile() there are unnecessary
white space changes and spaces at the end of the line.
Do you mean to say whitespaces in below text?
! * and absolute-ifying the path name if necessary.
! *
! * While parsing, it records if it has parsed persistent.auto.conf file.
! * This information can be used by the callers to ensure if the parameters
! * set by ALTER SYSTEM SET command will be effective.
*/
With Regards,
Amit Kapila.
--
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 for removng unused targets
Hi Alexander, I wrote: From: Tom Lane [mailto:t...@sss.pgh.pa.us] resjunk means that the target is not supposed to be output by the query. Since it's there at all, it's presumably referenced by ORDER BY or GROUP BY or DISTINCT ON, but the meaning of the flag doesn't depend on that. What you would need to do is verify that the target is resjunk and not used in any clause besides ORDER BY. I have not read your patch, but I rather imagine that what you've got now is that the parser checks this and sets the new flag for consumption far downstream. Why not just make the same check in the planner? I've created a patch using this approach. I've rebased the above patch against the latest head. Could you review the patch? If you have no objection, I'd like to mark the patch ready for committer. Sorry, I've had a cleanup of the patch. Please find attached the patch. Thanks, Best regards, Etsuro Fujita unused-targets-20130618-2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Memory leak in PL/pgSQL function which CREATE/SELECT/DROP a temporary table
On 18.06.2013 14:27, MauMau wrote: The cause of the memory increase appears to be CacheMemoryContext. When I attached to postgres with gdb and ran call MemoryContextStats(TopMemoryContext) several times, the size of CacheMemoryContext kept increasing. Hmm. I could repeat this, and it seems that the catcache for pg_statistic accumulates negative cache entries. Those slowly take up the memory. Seems that we should somehow flush those, when the table is dropped. Not sure how, but I'll take a look. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for REINDEX CONCURRENTLY
Hi,
On 2013-06-18 11:35:10 +0200, Andres Freund wrote:
Going to do some performance tests now.
Ok, so ran the worst case load I could think of and didn't notice
any relevant performance changes.
The test I ran was:
CREATE TABLE test_toast(id serial primary key, data text);
ALTER TABLE test_toast ALTER COLUMN data SET STORAGE external;
INSERT INTO test_toast(data) SELECT repeat('a', 8000) FROM generate_series(1,
20);
VACUUM FREEZE test_toast;
And then with that:
\setrandom id 1 20
SELECT id, substring(data, 1, 10) FROM test_toast WHERE id = :id;
Which should really stress the potentially added overhead since we're
doing many toast accesses, but always only fetch one chunk.
One other thing: Your latest patch forgot to adjust rules.out, so make
check didn't pass...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Bugfix and new feature for PGXS
I've rebased the current set of pending patches I had, to fix pgxs and added 2 new patches. Bugfixes have already been presented and sent in another thread, except the last one which only fix comment in pgxs.mk. The new feature consists in a new variable to allow the installation of contrib header files. A new variable MODULEHEADER can be used in extension Makefile to list the header files to install. The installation path default to $includedir/$includedir_contrib/$extension. 2 new variables are set to define directories: $includedir_contrib and $includedir_extension, the former default to include/contrib and the later to $includedir_contrib/$extension ($extension is the name of the extension). This allows for example to install hstore header and be able to include them in another extension like that: # include contrib/hstore/hstore.h For packagers of PostgreSQL, this allows to have a postgresql-X.Y-contrib-dev package. For developers of extension it's killing the copy-and-paste-this-function dance previously required. I've not updated contribs' Makfefile: I'm not sure what we want to expose. I've 2 other patches to write to automatize a bit more the detection of things to do when building with USE_PGXS, based on the layout. Better get a good consensus on this before writing them. Bugfix: 0001-fix-SHLIB_PREREQS-when-building-with-USE_PGXS.patch 0002-Create-data-directory-if-extension-when-required.patch 0003-set-VPATH-for-out-of-tree-extension-builds.patch 0004-adds-support-for-VPATH-with-USE_PGXS.patch 0006-Fix-suggested-layout-for-extension.patch New feature: 0005-Allows-extensions-to-install-header-file.patch I'll do a documentation patch based on what is accepted in HEAD and/or previous branches. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation From c23041f31b5a312702d79bbe759a56628f3e37e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Villemain?= ced...@2ndquadrant.fr Date: Tue, 28 May 2013 14:11:18 +0200 Subject: [PATCH 1/6] fix SHLIB_PREREQS when building with USE_PGXS commit 19e231b introduced SHLIB_PREREQS but failed to port that to PGXS build. The issue is that submake-* can not be built with PGXS, in this case they must check that expected files are present (and installed). Maybe it is better to only check if they have been built ? This fix the build of dblink and postgres_fdw (make USE_PGXS=1) --- src/Makefile.global.in | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 8bfb77d..c3c595e 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -415,13 +415,24 @@ libpq_pgport = -L$(top_builddir)/src/port -lpgport \ -L$(top_builddir)/src/common -lpgcommon $(libpq) endif - +# If PGXS is not defined, builds as usual: +# build dependancies required by SHLIB_PREREQS +# If the build is with PGXS, then any requirement is supposed to be already +# build and we just take care that the expected files exist +ifndef PGXS submake-libpq: $(MAKE) -C $(libpq_builddir) all +else +submake-libpq: $(libdir)/libpq.so ; +endif +ifndef PGXS submake-libpgport: $(MAKE) -C $(top_builddir)/src/port all $(MAKE) -C $(top_builddir)/src/common all +else +submake-libpgport: $(libdir)/libpgport.a $(libdir)/libpgcommon.a ; +endif .PHONY: submake-libpq submake-libpgport -- 1.7.10.4 From 3d3f4df6792c0e98b0a915b8704504f27738bf26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Villemain?= ced...@2ndquadrant.fr Date: Tue, 28 May 2013 14:17:04 +0200 Subject: [PATCH 2/6] Create data directory if extension when required There is a hack to link the regression data files from the srcdir to the builddir when doing 'make VPATH'. but it failed when used in conjunction with USE_PGXS and out-of-tree build of an extension. Issue is the absence of the data/ directory in the builddir. --- src/makefiles/pgxs.mk |1 + 1 file changed, 1 insertion(+) diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk index bbcfe04..6a19b0f 100644 --- a/src/makefiles/pgxs.mk +++ b/src/makefiles/pgxs.mk @@ -263,6 +263,7 @@ test_files_build := $(patsubst $(srcdir)/%, $(abs_builddir)/%, $(test_files_src) all: $(test_files_build) $(test_files_build): $(abs_builddir)/%: $(srcdir)/% + $(MKDIR_P) $(dir $@) ln -s $ $@ endif # VPATH -- 1.7.10.4 From 66b394ae867bde2ad968027f0708ae59a140d81b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Villemain?= ced...@2ndquadrant.fr Date: Tue, 28 May 2013 14:51:43 +0200 Subject: [PATCH 3/6] set VPATH for out-of-tree extension builds If the makefile is not in the current directory (where we launch 'make') then assume we are building out-of-src tree and set the VPATH to the directory of the first makefile... Thus it fixes: mkdir /tmp/a cd /tmp/a make -f extension_src/Makefile USE_PGXS=1 --- src/makefiles/pgxs.mk |9 + 1 file changed, 9 insertions(+)
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
2013-06-18 14:11 keltezéssel, Amit Kapila írta:
On Tuesday, June 18, 2013 3:26 PM Boszormenyi Zoltan wrote:
Hi,
review below.
Thanks for the review.
There are 2 options to proceed for this patch for 9.4
1. Upload the SET PERSISTENT syntax patch for coming CF by fixing
existing
review comments
2. Implement new syntax ALTER SYSTEM as proposed in below mail
Could you suggest me what could be best way to proceed for this
patch?
I'm still in favor of some syntax involving ALTER, because it's still
true that this behaves more like the existing GUC-setting commands
that use ALTER (which change configuration for future sessions)
rather
the ones that use SET (which change the current settings for some
period of time).
I will change the patch as per below syntax if there are no objections:
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value'};
Modified patch contains:
1. Syntax implemented is
ALTER SYSTEM SET configuration_parameter {TO | =} {value, | 'value' |
DEFAULT};
If user specifies DEFAULT, it will remove entry from auto conf file.
2. File name to store settings set by ALTER SYSTEM command is still
persistent.auto.conf
3. Added a new page for Alter System command in docs. In compatability
section, I had mentioned that
this statement is an PostgresQL extension. I had tried to search in
SQL-92 spec but couldn't find such command.
4. During test of this patch, I had observed one problem for PGC_BACKEND
parameters like log_connections.
If I change these parameters directly in postgresql.conf and then do
pg_reload_conf() and reconnect, it will
still show the old value. This is observed only on WINDOWS. I will
investigate this problem and update you.
Due to this problem, I had removed few test cases.
I can't reproduce this error under Linux (Fedora 19/x86_64).
The bug might be somewhere else and not caused by your patch
if manually adding/removing log_connections = on from postgresql.conf
exhibits the same behaviour under Windows. (If I read it correctly,
you tested it exactly this way.) Does the current GIT version behave
the same way?
Yes, the current Git has problem which I had reported in below mail:
http://www.postgresql.org/message-id/009801ce68f7$3a746340$af5d29c0$@kapila@
huawei.com
This problem is not related to this patch, it occurs only on WINDOWS or
under EXEC_BACKEND flag.
I think we can discuss this problem in above mail thread.
* Have all the bases been covered?
According to the above note under Windows, not yet.
The name persistent.auto.conf is not yet set in stone.
postgresql.auto.conf might be better.
I think, the decision of name, we can leave to committer with below
possibilities,
as it is very difficult to build consensus on any particular name.
Auto.conf
System.auto.conf
Postgresql.auto.conf
Persistent.auto.conf
Apply the patch, compile it and test:
* Does it follow the project coding guidelines?
Mostly, yes. Some nits, though. At some places, you do:
- ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head, tail);
+ ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head,
tail,NULL);
I think you mean ParseConfigFp()?
Sorry, yes.
without a space between the last comma and the NULL keyword.
Also, in the comment above ParseConfigFile() there are unnecessary
white space changes and spaces at the end of the line.
Do you mean to say whitespaces in below text?
! * and absolute-ifying the path name if necessary.
! *
! * While parsing, it records if it has parsed persistent.auto.conf file.
! * This information can be used by the callers to ensure if the parameters
! * set by ALTER SYSTEM SET command will be effective.
*/
Yes.
Anyway, both would be fixed by a pgindent run. (I think.)
With Regards,
Amit Kapila.
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for fail-back without fresh backup
On Tuesday, June 18, 2013, Amit Kapila wrote: On Tuesday, June 18, 2013 12:18 AM Sawada Masahiko wrote: On Sun, Jun 16, 2013 at 2:00 PM, Amit kapila amit.kap...@huawei.comjavascript:; wrote: On Saturday, June 15, 2013 8:29 PM Sawada Masahiko wrote: On Sat, Jun 15, 2013 at 10:34 PM, Amit kapila amit.kap...@huawei.com javascript:; wrote: On Saturday, June 15, 2013 1:19 PM Sawada Masahiko wrote: On Fri, Jun 14, 2013 at 10:15 PM, Amit Kapila amit.kap...@huawei.com javascript:; wrote: On Friday, June 14, 2013 2:42 PM Samrat Revagade wrote: Hello, We have already started a discussion on pgsql-hackers for the problem of taking fresh backup during the failback operation here is the link for that: http://www.postgresql.org/message-id/CAF8Q-Gxg3PQTf71NVECe- 6OzRaew5pWhk7yQtb jgwrfu513...@mail.gmail.com javascript:; Let me again summarize the problem we are trying to address. How will you take care of extra WAL on old master during recovery. If it plays the WAL which has not reached new-master, it can be a problem. you means that there is possible that old master's data ahead of new master's data. I mean to say is that WAL of old master can be ahead of new master. I understood that data files of old master can't be ahead, but I think WAL can be ahead. so there is inconsistent data between those server when fail back. right? if so , there is not possible inconsistent. because if you use GUC option as his propose (i.g., failback_safe_standby_mode = remote_flush), when old master is working fine, all file system level changes aren't done before WAL replicated. Would the propose patch will take care that old master's WAL is also not ahead in some way? If yes, I think i am missing some point. yes it will happen that old master's WAL ahead of new master's WAL as you said. but I think that we can solve them by delete all WAL file when old master starts as new standby. I think ideally, it should reset WAL location at the point where new master has forrked off. In such a scenario it would be difficult for user who wants to get a dump of some data in old master which hasn't gone to new master. I am not sure if such a need is there for real users, but if it is there, then providing this solution will have some drawbacks. I think that we can dumping data before all WAL files deleting. All WAL files deleting is done when old master starts as new standby. Can we dump data without starting server? Sorry I made a mistake. We can't it. this proposing patch need to be able to also handle such scenario in future. Regards, --- Sawada Masahiko -- Regards, --- Sawada Masahiko
Re: [HACKERS] Add regression tests for SET xxx
On 18 June 2013 17:29, Kevin Grittner kgri...@ymail.com wrote: Szymon Guz mabew...@gmail.com wrote: I've checked the patch. Applies cleanly. Tests pass this time :) Could you add me as a reviewer to commitfest website, set this patch a reviewed and add this email to the patch history? I cannot login to the commitfest app, there is some bug with that. It sounded like you felt this was Ready for Committer, so I set it that way. Let me know if you don't think it's to that point yet. Hi Kevin, yes, that's what I was thinking about. Thanks, Szymon
Re: [HACKERS] Support for REINDEX CONCURRENTLY
On Tue, Jun 18, 2013 at 10:53 AM, Michael Paquier michael.paqu...@gmail.com wrote: An updated patch for the toast part is attached. On Tue, Jun 18, 2013 at 3:26 AM, Fujii Masao masao.fu...@gmail.com wrote: Here are the review comments of the removal_of_reltoastidxid patch. I've not completed the review yet, but I'd like to post the current comments before going to bed ;) *** a/src/backend/catalog/system_views.sql -pg_stat_get_blocks_fetched(X.oid) - -pg_stat_get_blocks_hit(X.oid) AS tidx_blks_read, -pg_stat_get_blocks_hit(X.oid) AS tidx_blks_hit +pg_stat_get_blocks_fetched(X.indrelid) - +pg_stat_get_blocks_hit(X.indrelid) AS tidx_blks_read, +pg_stat_get_blocks_hit(X.indrelid) AS tidx_blks_hit ISTM that X.indrelid indicates the TOAST table not the TOAST index. Shouldn't we use X.indexrelid instead of X.indrelid? Indeed good catch! We need in this case the statistics on the index and here I used the table OID. Btw, I also noticed that as multiple indexes may be involved for a given toast relation, it makes sense to actually calculate tidx_blks_read and tidx_blks_hit as the sum of all stats of the indexes. Yep. You seem to need to change X.indexrelid to X.indrelid in GROUP clause. Otherwise, you may get two rows of the same table from pg_statio_all_tables. doc/src/sgml/diskusage.sgml There will be one index on the acronymTOAST/ table, if present. + table (see xref linkend=storage-toast). There will be one valid index + on the acronymTOAST/ table, if present. There also might be indexes When I used gdb and tracked the code path of concurrent reindex patch, I found it's possible that more than one *valid* toast indexes appear. Those multiple valid toast indexes are viewable, for example, from pg_indexes. I'm not sure whether this is the bug of concurrent reindex patch. But if it's not, you seem to need to change the above description again. *** a/src/bin/pg_dump/pg_dump.c + SELECT c.reltoastrelid, t.indexrelid FROM pg_catalog.pg_class c LEFT JOIN - pg_catalog.pg_class t ON (c.reltoastrelid = t.oid) - WHERE c.oid = '%u'::pg_catalog.oid;, + pg_catalog.pg_index t ON (c.reltoastrelid = t.indrelid) + WHERE c.oid = '%u'::pg_catalog.oid AND t.indisvalid + LIMIT 1, Is there the case where TOAST table has more than one *valid* indexes? I just rechecked the patch and is answer is no. The concurrent index is set as valid inside the same transaction as swap. So only the backend performing the swap will be able to see two valid toast indexes at the same time. According to my quick gdb testing, this seems not to be true Regards, -- Fujii Masao -- 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] Support for REINDEX CONCURRENTLY
On Tue, Jun 18, 2013 at 9:54 PM, Andres Freund and...@2ndquadrant.com wrote:
Hi,
On 2013-06-18 11:35:10 +0200, Andres Freund wrote:
Going to do some performance tests now.
Ok, so ran the worst case load I could think of and didn't notice
any relevant performance changes.
The test I ran was:
CREATE TABLE test_toast(id serial primary key, data text);
ALTER TABLE test_toast ALTER COLUMN data SET STORAGE external;
INSERT INTO test_toast(data) SELECT repeat('a', 8000) FROM generate_series(1,
20);
VACUUM FREEZE test_toast;
And then with that:
\setrandom id 1 20
SELECT id, substring(data, 1, 10) FROM test_toast WHERE id = :id;
Which should really stress the potentially added overhead since we're
doing many toast accesses, but always only fetch one chunk.
Sounds really good!
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add regression tests for SET xxx
Szymon Guz mabew...@gmail.com wrote: I've checked the patch. Applies cleanly. Tests pass this time :) Could you add me as a reviewer to commitfest website, set this patch a reviewed and add this email to the patch history? I cannot login to the commitfest app, there is some bug with that. It sounded like you felt this was Ready for Committer, so I set it that way. Let me know if you don't think it's to that point yet. -- Kevin Grittner 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] Patch for fast gin cache performance improvement
Ian Link i...@ilink.io wrote: This patch contains a performance improvement for the fast gin cache. Our test queries improve from about 0.9 ms to 0.030 ms. Impressive! Thanks for reading and considering this patch! Congratulations on your first PostgreSQL patch! To get it scheduled for review, please add it to this page: https://commitfest.postgresql.org/action/commitfest_view/open You will need to get a community login (if you don't already have one), but that is a quick and painless process. Choose an appropriate topic (like Performance) and reference the message ID of the email to which you attached the patch. Don't worry about the fields for reviewers, committer, or date closed. Sorry for the administrative overhead, but without it things can fall through the cracks. You can find an overview of the review process with links to more detail here: http://wiki.postgresql.org/wiki/CommitFest Thanks for contributing! -- Kevin Grittner 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] SET work_mem = '1TB';
On Tue, Jun 18, 2013 at 1:06 PM, Jeff Janes jeff.ja...@gmail.com wrote: On Tuesday, May 21, 2013, Simon Riggs wrote: I worked up a small patch to support Terabyte setting for memory. Which is OK, but it only works for 1TB, not for 2TB or above. I've incorporated my review into a new version, attached. Added TB to the docs, added the macro KB_PER_TB, and made show to print 1TB rather than 1024GB. Looks good to me. But I found you forgot to change postgresql.conf.sample, so I changed it and attached the updated version of the patch. Barring any objection to this patch and if no one picks up this, I will commit this. Regards, -- Fujii Masao terabyte_work_mem_fujii_v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for fast gin cache performance improvement
Kevin Grittner kgri...@ymail.com wrote: You will need to get a community login (if you don't already have one), but that is a quick and painless process. Oops -- we seem to have a problem with new community logins at the moment, which will hopefully be straightened out soon. You might want to wait a few days if you don't already have a login. -- Kevin Grittner 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
[HACKERS] dump difference between 9.3 and master after upgrade
As I was updating my cross version upgrade tester to include support for the 9.3 branch, I noted this dump difference between the dump of the original 9.3 database and the dump of the converted database, which looks odd. Is it correct? cheers andrew --- /home/bf/bfr/root/upgrade/HEAD/origin-REL9_3_STABLE.sql 2013-06-18 08:45:22.518761202 -0400 +++ /home/bf/bfr/root/upgrade/HEAD/converted-REL9_3_STABLE-to-HEAD.sql 2013-06-18 08:46:01.648782111 -0400 @@ -385,6 +385,7 @@ -- CREATE FOREIGN TABLE ft1 ( +pg.dropped.1 integer, c1 integer NOT NULL, c2 integer NOT NULL, c3 text, @@ -413,6 +414,7 @@ CREATE FOREIGN TABLE ft2 ( c1 integer NOT NULL, c2 integer NOT NULL, +pg.dropped.3 integer, c3 text, c4 timestamp with time zone, c5 timestamp without time zone, -- 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 for removng unused targets
Hi Etsuro! On Tue, Jun 18, 2013 at 4:15 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jpwrote: Hi Alexander, I wrote: From: Tom Lane [mailto:t...@sss.pgh.pa.us] resjunk means that the target is not supposed to be output by the query. Since it's there at all, it's presumably referenced by ORDER BY or GROUP BY or DISTINCT ON, but the meaning of the flag doesn't depend on that. What you would need to do is verify that the target is resjunk and not used in any clause besides ORDER BY. I have not read your patch, but I rather imagine that what you've got now is that the parser checks this and sets the new flag for consumption far downstream. Why not just make the same check in the planner? I've created a patch using this approach. I've rebased the above patch against the latest head. Could you review the patch? If you have no objection, I'd like to mark the patch ready for committer. Sorry, I've had a cleanup of the patch. Please find attached the patch. I've checked the attached patch. It looks good for me. No objection to mark it ready for committer. -- With best regards, Alexander Korotkov.
[HACKERS] Git-master regression failure
Hi All. I just subscribed to RRReviewers (that should be pronounce with a nice rolling r-r-reviewers, right?) As part of my getting up to speed, I tried to build and run test on the current master 073d7cb513f5de44530f4bdbaaa4b5d4cce5f984 Basically I did: 1) Clone into new dir 2) ./configure --enable-debug --enable-cassert --with-pgport=5499 --prefix=$(realpath ../root) 3) make -j4 4) maje -j4 check And expecting in 4) to get all test passed, I was surprised to see that an index-test failed. I run (Gentoo) Linux x86_64, gcc-4.7.3. Any ideas what might have happened? Svenne *** /home/sk/tmp/pgsql-test/source/src/test/regress/expected/create_index.out 2013-06-18 17:38:03.411169360 +0200 --- /home/sk/tmp/pgsql-test/source/src/test/regress/results/create_index.out 2013-06-18 18:36:18.178373329 +0200 *** *** 2673,2679 WHERE f1 'WA' and id 1000 and f1 ~~ 'YX'; count --- ! 97 (1 row) -- --- 2673,2679 WHERE f1 'WA' and id 1000 and f1 ~~ 'YX'; count --- ! 99 (1 row) -- == -- 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_filedump 9.3: checksums (and a few other fixes)
On Thu, 2013-06-13 at 20:09 -0400, Tom Lane wrote:
What I propose we do about this is reduce backend/storage/page/checksum.c
to something like
#include postgres.h
#include storage/checksum.h
#include storage/checksum_impl.h
Attached a new diff for pg_filedump that makes use of the above change.
I'm not sure what the resolution of Alvaro's concern was, so I left the
flag reporting the same as the previous patch.
Regards,
Jeff Davis
Common subdirectories: pg_filedump-9.2.0/.deps and pg_filedump-9.3.0j/.deps
diff -Nc pg_filedump-9.2.0/Makefile pg_filedump-9.3.0j/Makefile
*** pg_filedump-9.2.0/Makefile 2012-03-12 09:02:44.0 -0700
--- pg_filedump-9.3.0j/Makefile 2013-06-18 09:14:42.442220848 -0700
***
*** 1,7
# View README.pg_filedump first
# note this must match version macros in pg_filedump.h
! FD_VERSION=9.2.0
CC=gcc
CFLAGS=-g -O -Wall -Wmissing-prototypes -Wmissing-declarations
--- 1,7
# View README.pg_filedump first
# note this must match version macros in pg_filedump.h
! FD_VERSION=9.3.0
CC=gcc
CFLAGS=-g -O -Wall -Wmissing-prototypes -Wmissing-declarations
diff -Nc pg_filedump-9.2.0/pg_filedump.c pg_filedump-9.3.0j/pg_filedump.c
*** pg_filedump-9.2.0/pg_filedump.c 2012-03-12 08:58:31.0 -0700
--- pg_filedump-9.3.0j/pg_filedump.c 2013-06-18 09:25:42.438208300 -0700
***
*** 26,31
--- 26,37
#include utils/pg_crc_tables.h
+ // checksum_impl.h uses Assert, which doesn't work outside the server
+ #undef Assert
+ #define Assert(X)
+
+ #include storage/checksum_impl.h
+
// Global variables for ease of use mostly
static FILE *fp = NULL; // File to dump or format
static char *fileName = NULL; // File name for display
***
*** 40,51
static void DisplayOptions (unsigned int validOptions);
static unsigned int ConsumeOptions (int numOptions, char **options);
static int GetOptionValue (char *optionString);
! static void FormatBlock ();
static unsigned int GetBlockSize ();
static unsigned int GetSpecialSectionType (Page page);
static bool IsBtreeMetaPage(Page page);
static void CreateDumpFileHeader (int numOptions, char **options);
! static int FormatHeader (Page page);
static void FormatItemBlock (Page page);
static void FormatItem (unsigned int numBytes, unsigned int startIndex,
unsigned int formatAs);
--- 46,57
static void DisplayOptions (unsigned int validOptions);
static unsigned int ConsumeOptions (int numOptions, char **options);
static int GetOptionValue (char *optionString);
! static void FormatBlock (BlockNumber blkno);
static unsigned int GetBlockSize ();
static unsigned int GetSpecialSectionType (Page page);
static bool IsBtreeMetaPage(Page page);
static void CreateDumpFileHeader (int numOptions, char **options);
! static int FormatHeader (Page page, BlockNumber blkno);
static void FormatItemBlock (Page page);
static void FormatItem (unsigned int numBytes, unsigned int startIndex,
unsigned int formatAs);
***
*** 288,293
--- 294,304
SET_OPTION (itemOptions, ITEM_DETAIL, 'i');
break;
+ // Verify block checksums
+ case 'k':
+ SET_OPTION (blockOptions, BLOCK_CHECKSUMS, 'k');
+ break;
+
// Interpret items as standard index values
case 'x':
SET_OPTION (itemOptions, ITEM_INDEX, 'x');
***
*** 555,561
// Dump out a formatted block header for the requested block
static int
! FormatHeader (Page page)
{
int rc = 0;
unsigned int headerBytes;
--- 566,572
// Dump out a formatted block header for the requested block
static int
! FormatHeader (Page page, BlockNumber blkno)
{
int rc = 0;
unsigned int headerBytes;
***
*** 609,623
Block: Size %4d Version %4uUpper%4u (0x%04hx)\n
LSN: logid %6d recoff 0x%08x Special %4u (0x%04hx)\n
Items: %4d Free Space: %4u\n
! TLI: 0x%04x Prune XID: 0x%08x Flags: 0x%04x (%s)\n
Length (including item array): %u\n\n,
pageOffset, pageHeader-pd_lower, pageHeader-pd_lower,
(int) PageGetPageSize (page), blockVersion,
pageHeader-pd_upper, pageHeader-pd_upper,
! pageLSN.xlogid, pageLSN.xrecoff,
pageHeader-pd_special, pageHeader-pd_special,
maxOffset, pageHeader-pd_upper - pageHeader-pd_lower,
! pageHeader-pd_tli, pageHeader-pd_prune_xid,
pageHeader-pd_flags, flagString,
headerBytes);
--- 620,634
Block: Size %4d Version %4uUpper%4u (0x%04hx)\n
LSN: logid %6d recoff 0x%08x Special %4u (0x%04hx)\n
Items: %4d Free Space: %4u\n
! Checksum: %05hu Prune XID: 0x%08x Flags: 0x%04x (%s)\n
Length (including item array): %u\n\n,
pageOffset, pageHeader-pd_lower, pageHeader-pd_lower,
(int) PageGetPageSize (page), blockVersion,
pageHeader-pd_upper, pageHeader-pd_upper,
! (uint32) (pageLSN 32),
Re: [HACKERS] A minor correction in comment in heaptuple.c
Andres Freund and...@2ndquadrant.com wrote: On 2013-06-18 05:21:15 -0400, D'Arcy J.M. Cain wrote: On Tue, 18 Jun 2013 11:01:28 +0200 Andres Freund and...@2ndquadrant.com wrote: /* * return true if attnum is out of range according to the tupdesc */ if (attnum tupleDesc-natts) return true; I think the comment is more meaningfull before the change since it tells us how nonexisting columns are interpreted. I think that the comment is bad either way. Comments should explain the code, not repeat it. The above is not far removed from... return 5; /* return the number 5 */ I agree with this -- the comment as it stands adds no information to what is obvious from a glance at the code, and may waste the time it takes to mentally resolve the discrepancy between return NULL in the comment and return true; in the statement. Unless it adds information, we'd be better off deleting the comment. How about check if attnum is out of range according to the tupdesc instead? I can't follow. Minus the word 'NULL' - which carries meaning - your suggested comment pretty much is the same as the existing comment except that you use 'check' instead of 'return'. The word indicate might waste a few milliseconds less in the double-take; but better would be some explanation of why you might have an attnum value greater than the maximum, and why the right thing to do is indicate NULL rather than throwing an error. -- Kevin Grittner 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] extensible external toast tuple support
On Tue, Jun 18, 2013 at 1:58 AM, Andres Freund and...@2ndquadrant.comwrote: On 2013-06-18 00:56:17 -0700, Hitoshi Harada wrote: On Fri, Jun 14, 2013 at 4:06 PM, Andres Freund and...@2ndquadrant.com wrote: Here's the updated version. It shouldn't contain any obvious WIP pieces anymore, although I think it needs some more documentation. I am just not sure where to add it yet, postgres.h seems like a bad place :/ I have a few comments and questions after reviewing this patch. Cool! - heap_tuple_fetch_attr doesn't need to be updated to reflect ONDISK macro? It calls toast_fetch_datum() for any kind of external datum, so it should be fine since ONDISK is handled in there. toast_fetch_datum doesn't expect the input is INDIRECT. At least I see the code path in the same file around toast_insert_or_update() where we have a chance to (possibly accidentally) try to fetch ONDISK toasted value from non-ONDISK datum. - -1 from me to use enum for tag types, as I don't think it needs to be. This looks more like other kind macros such as relkind, but I know there's pros/cons Well, relkind cannot easily be a enum because it needs to be stored in a char field. I like using enums because it gives you the chance of using switch()es at some point and getting warned about missed cases. Why do you dislike it? I put -1 just because it doesn't have to be now. If you argue relkind is char field, tag is also uint8. - don't we need cast for tag value comparison in VARTAG_SIZE macro, since tag is unit8 and enum is signed int? I think it should be fine (and the compiler doesn't warn) due to the integer promotion rules. - Is there better way to represent ONDISK size, instead of magic number 18? I'd suggest constructing it with sizeof(varatt_external). I explicitly did not want to do that, since the numbers really don't have anything to do with the struct size anymore. Otherwise the next person reading that part will be confused because there's a 8byte struct with the enum value 1. Or somebody will try adding another type of external tuple that also needs 18 bytes by copying the sizeof(). Which will fail in ugly, non-obvious ways. Sounds reasonable. I was just confused when I looked at it first. Other than that, the patch applies fine and make check runs, though I don't think the new code path is exercised well, as no one is creating INDIRECT datum yet. Yea, I only use the API in the changeset extraction patch. That actually worries me to. But I am not sure where to introduce usage of it in core without making the patchset significantly larger. I was thinking of adding an option to heap_form_tuple/heap_fill_tuple to allow it to reference _4B Datums instead of copying them, but that would require quite some adjustments on the callsites. Maybe you can create a user-defined function that creates such datum for testing, just in order to demonstrate it works fine. Also, I wonder how we could add more compression in datum, as well as how we are going to add more compression options in database. I'd love to see pluggability here, as surely the core cannot support dozens of different compression algorithms, but because the data on disk is critical and we cannot do anything like user defined functions. The algorithms should be optional builtin so that once the system is initialized the the plugin should not go away. Anyway pluggable compression is off-topic here. Separate patchset by now, yep ;). I just found. Will look into it. Thanks,
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Tue, 18 Jun 2013 11:38:45 +0200 Andres Freund and...@2ndquadrant.com wrote: How about check if attnum is out of range according to the tupdesc instead? I can't follow. Minus the word 'NULL' - which carries meaning - your suggested comment pretty much is the same as the existing comment except that you use 'check' instead of 'return'. The difference is that I say what the purpose of the function is but don't say what it actually returns. The code itself does that. Original: /* * return NULL if attnum is out of range according to the tupdesc */ Obviously wrong so it should be changed. As for the exact wording, flip a coin and get the bikeshed painted. It's not all that critical. You could probably leave out the comment altogether. The code is pretty short and self explanatory. Perhaps the comment should explain why we don't test for negative numbers. I assume that that's an impossible situation. -- D'Arcy J.M. Cain da...@druid.net | Democracy is three wolves http://www.druid.net/darcy/| and a sheep voting on +1 416 788 2246 (DoD#0082)(eNTP) | what's for dinner. IM: da...@vex.net, VOIP: sip:da...@vex.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] A minor correction in comment in heaptuple.c
On 2013-06-18 13:14:30 -0400, D'Arcy J.M. Cain wrote: On Tue, 18 Jun 2013 11:38:45 +0200 Andres Freund and...@2ndquadrant.com wrote: How about check if attnum is out of range according to the tupdesc instead? I can't follow. Minus the word 'NULL' - which carries meaning - your suggested comment pretty much is the same as the existing comment except that you use 'check' instead of 'return'. The difference is that I say what the purpose of the function is but don't say what it actually returns. The code itself does that. Original: /* * return NULL if attnum is out of range according to the tupdesc */ Obviously wrong so it should be changed. The NULL refers to the *meaning* of the function (remember, it's called slot_attisnull) . Which is to test whether an attribute is null. Not to a C NULL. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] LEFT JOIN LATERAL can remove rows from LHS
Maybe I am misunderstanding how LATERAL is supposed to work, but my expectation is that doing a LEFT JOIN should not remove rows from the LHS. I would expect all of the following select queries would return a single row, but that isn't the case: CREATE TABLE i (n integer); CREATE TABLE j (n integer); INSERT INTO i VALUES (10); SELECT * FROM i LEFT JOIN LATERAL (SELECT * FROM j) j ON true; n | n +--- 10 | (1 row) SELECT * FROM i LEFT JOIN LATERAL (SELECT * FROM j WHERE (i.n = j.n)) j ON true; n | n ---+--- (0 rows) INSERT INTO j VALUES (10); SELECT * FROM i LEFT JOIN LATERAL (SELECT * FROM j WHERE (i.n = j.n)) j ON true; n | n + 10 | 10 (1 row) SELECT * FROM i LEFT JOIN LATERAL (SELECT * FROM j WHERE (i.n = j.n)) j ON false; n | n ---+--- (0 rows) Is the error in PostgreSQL or my understanding of LATERAL subqueries? Please CC me when responding as I don't currently subscribe to the list. Thanks, Jeremy -- 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] ASYNC Privileges proposal
I had a quick play to see what might be involved [attached], and would like to hear people thoughts; good idea, bad idea, not like that! etc I question the usefulness of allowing listen/notify to be restricted to an entire class of users. The granularity of this seems too broad, though I am not sure if allowing message to be sent to a specific user is easily achievable. Well, if we're going to have privs on LISTEN/NOTIFY at all, they should be on specific message bands, i.e.: REVOKE LISTEN ON 'cacheupdates' FROM PUBLIC; GRANT LISTEN ON 'cacheupdates' TO webuser; GRANT LISTEN ON ALL TO admin; I can certainly see wanting this, but it'd be a great deal more sophisticated than what Chris has proposed. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Tue, 18 Jun 2013 19:19:40 +0200 Andres Freund and...@2ndquadrant.com wrote: The NULL refers to the *meaning* of the function (remember, it's called slot_attisnull) . Which is to test whether an attribute is null. Not to a C NULL. Actually, the comment is not for the function. It only describes the two lines that follow. In C the string NULL is commonly a reference to C's NULL value. Anyone reading C code can be excused for assuming that if it isn't otherwise clear. How about Indicate that the attribute is NULL if out of range...? Although, the more I think about it, the more I think that the comment is both confusing and superfluous. The code itself is much clearer. -- D'Arcy J.M. Cain da...@druid.net | Democracy is three wolves http://www.druid.net/darcy/| and a sheep voting on +1 416 788 2246 (DoD#0082)(eNTP) | what's for dinner. IM: da...@vex.net, VOIP: sip:da...@vex.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] SET work_mem = '1TB';
On 18 June 2013 17:10, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Jun 18, 2013 at 1:06 PM, Jeff Janes jeff.ja...@gmail.com wrote: On Tuesday, May 21, 2013, Simon Riggs wrote: I worked up a small patch to support Terabyte setting for memory. Which is OK, but it only works for 1TB, not for 2TB or above. I've incorporated my review into a new version, attached. Added TB to the docs, added the macro KB_PER_TB, and made show to print 1TB rather than 1024GB. Looks good to me. But I found you forgot to change postgresql.conf.sample, so I changed it and attached the updated version of the patch. Barring any objection to this patch and if no one picks up this, I will commit this. In truth, I hadn't realised somebody had added this to the CF. It was meant to be an exploration and demonstration that further work was/is required rather than a production quality submission. AFAICS it is still limited to '1 TB' only... Thank you both for adding to this patch. Since you've done that, it seems churlish of me to interrupt that commit. I will make a note to extend the support to higher values of TBs later. -- 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] SET work_mem = '1TB';
In truth, I hadn't realised somebody had added this to the CF. It was meant to be an exploration and demonstration that further work was/is required rather than a production quality submission. AFAICS it is still limited to '1 TB' only... At the beginning of the CF, I do a sweep of patch files emailed to -hackers and not in the CF. I believe there were three such of yours, take a look at the CF list. Like I said, better to track them unnecessarily than to lose them. Thank you both for adding to this patch. Since you've done that, it seems churlish of me to interrupt that commit. Well, I think that someone needs to actually test doing a sort with, say, 100GB of RAM and make sure it doesn't crash. Anyone have a machine they can try that on? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SET work_mem = '1TB';
* Josh Berkus (j...@agliodbs.com) wrote: Well, I think that someone needs to actually test doing a sort with, say, 100GB of RAM and make sure it doesn't crash. Anyone have a machine they can try that on? It can be valuable to bump up work_mem well beyond the amount of system memory actually available on the system to get the 'right' plan to be chosen (which often ends up needing much less actual memory to run). I've used that trick on a box w/ 512GB of RAM and had near-100G PG backend processes which were doing hashjoins. Don't think I've ever had it try doing a sort w/ a really big work_mem. Thanks, Stephen signature.asc Description: Digital signature
Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
Amit, I think, the decision of name, we can leave to committer with below possibilities, as it is very difficult to build consensus on any particular name. Auto.conf System.auto.conf Postgresql.auto.conf Persistent.auto.conf Reasons for auto.conf as a choice above all of the previous: 1) short 2) significantly different from postgresql.conf 3) near the beginning of the alphabet, so it should sort near the top if there are other files in conf/ directory -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SET work_mem = '1TB';
On 18 June 2013 18:45, Josh Berkus j...@agliodbs.com wrote: In truth, I hadn't realised somebody had added this to the CF. It was meant to be an exploration and demonstration that further work was/is required rather than a production quality submission. AFAICS it is still limited to '1 TB' only... At the beginning of the CF, I do a sweep of patch files emailed to -hackers and not in the CF. I believe there were three such of yours, take a look at the CF list. Like I said, better to track them unnecessarily than to lose them. Thanks. Please delete the patch marked Batch API for After Triggers. All others are submissions by me. -- 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] extensible external toast tuple support
On 2013-06-18 10:13:39 -0700, Hitoshi Harada wrote: On Tue, Jun 18, 2013 at 1:58 AM, Andres Freund and...@2ndquadrant.comwrote: On 2013-06-18 00:56:17 -0700, Hitoshi Harada wrote: On Fri, Jun 14, 2013 at 4:06 PM, Andres Freund and...@2ndquadrant.com wrote: Here's the updated version. It shouldn't contain any obvious WIP pieces anymore, although I think it needs some more documentation. I am just not sure where to add it yet, postgres.h seems like a bad place :/ I have a few comments and questions after reviewing this patch. Cool! - heap_tuple_fetch_attr doesn't need to be updated to reflect ONDISK macro? It calls toast_fetch_datum() for any kind of external datum, so it should be fine since ONDISK is handled in there. toast_fetch_datum doesn't expect the input is INDIRECT. At least I see the code path in the same file around toast_insert_or_update() where we have a chance to (possibly accidentally) try to fetch ONDISK toasted value from non-ONDISK datum. Hm. Yes. I don't think that's really possible in any codepath I have thought of - or tested - but that's not a good reason not to make it robust. - -1 from me to use enum for tag types, as I don't think it needs to be. This looks more like other kind macros such as relkind, but I know there's pros/cons Well, relkind cannot easily be a enum because it needs to be stored in a char field. I like using enums because it gives you the chance of using switch()es at some point and getting warned about missed cases. Why do you dislike it? I put -1 just because it doesn't have to be now. If you argue relkind is char field, tag is also uint8. Well, but to expose it to sql you need a numeric value that actually translates to a visible ascii character. Maybe you can create a user-defined function that creates such datum for testing, just in order to demonstrate it works fine. Hm. Will try whether I can think of something not completely pointless ;) Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A minor correction in comment in heaptuple.c
D'Arcy J.M. Cain da...@druid.net Although, the more I think about it, the more I think that the comment is both confusing and superfluous. The code itself is much clearer. Seriously, if there is any comment there at all, it should be a succinct explanation for why we didn't do this (which passes `make check-world`): --- a/src/backend/access/common/heaptuple.c +++ b/src/backend/access/common/heaptuple.c @@ -1323,6 +1323,8 @@ slot_attisnull(TupleTableSlot *slot, int attnum) HeapTuple tuple = slot-tts_tuple; TupleDesc tupleDesc = slot-tts_tupleDescriptor; + Assert(attnum = tupleDesc-natts); + /* * system attributes are handled by heap_attisnull */ @@ -1342,12 +1344,6 @@ slot_attisnull(TupleTableSlot *slot, int attnum) return slot-tts_isnull[attnum - 1]; /* - * return NULL if attnum is out of range according to the tupdesc - */ - if (attnum tupleDesc-natts) - return true; - - /* * otherwise we had better have a physical tuple (tts_nvalid should equal * natts in all virtual-tuple cases) */ -- Kevin Grittner 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] Memory leak in PL/pgSQL function which CREATE/SELECT/DROP a temporary table
On 18.06.2013 15:48, Heikki Linnakangas wrote: On 18.06.2013 14:27, MauMau wrote: The cause of the memory increase appears to be CacheMemoryContext. When I attached to postgres with gdb and ran call MemoryContextStats(TopMemoryContext) several times, the size of CacheMemoryContext kept increasing. Hmm. I could repeat this, and it seems that the catcache for pg_statistic accumulates negative cache entries. Those slowly take up the memory. Digging a bit deeper, this is a rather common problem with negative catcache entries. In general, nothing stops you from polluting the cache with as many negative cache entries as you like. Just do select * from table_that_doesnt_exist for as many non-existent table names as you want, for example. Those entries are useful at least in theory; they speed up throwing the error the next time you try to query the same non-existent table. But there is a crucial difference in this case; the system created a negative cache entry for the pg_statistic row of the table, but once the relation is dropped, the cache entry keyed on the relation's OID, is totally useless. It should be removed. We have this problem with a few other catcaches too, which have what is effectively a foreign key relationship with another catalog. For example, the RELNAMENSP catcache is keyed on pg_class.relname, pg_class.relnamespace, yet any negative entries are not cleaned up when the schema is dropped. If you execute this repeatedly in a session: CREATE SCHEMA foo; SELECT * from foo.invalid; -- throws an error DROP SCHEMA foo; it will leak similarly to the original test case, but this time the leak is into the RELNAMENSP catcache. To fix that, I think we'll need to teach the catalog cache about the relationships between the caches. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SET work_mem = '1TB';
On 06/18/2013 10:59 AM, Simon Riggs wrote: Thanks. Please delete the patch marked Batch API for After Triggers. All others are submissions by me. The CF app doesn't permit deletion of patches, so I marked it returned with feedback. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Git-master regression failure
Svenne Krap svenne.li...@krap.dk wrote: current master 073d7cb513f5de44530f4bdbaaa4b5d4cce5f984 I was surprised to see that an index-test failed. It works for me. Could you paste or attach some detail? -- Kevin Grittner 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] XLogInsert scaling, revisited
Hi Heikki, I am getting conflicts applying version 22 of this patch to 9.4dev. Could you rebase? Does anyone know of an easy way to apply an external patch through git, so I can get git-style merge conflict markers, rather than getting patch's reject file? Cheers, Jeff
Re: [HACKERS] Git-master regression failure
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 18-06-2013 18:40, Svenne Krap wrote: Any ideas what might have happened? After doing some more digging... My laptop (which runs PostgreSQL 9.2.4 on x86_64-pc-linux-gnu, compiled by x86_64-pc-linux-gnu-gcc (Gentoo 4.7.3 p1.0, pie-0.5.5) 4.7.3, 64-bit) also returns 99, if I - - run the CREATE TABLE tenk1 (from the git-master) - - load data from tenk.data (from git-master) - - run the offending part of the create_index.sql (also from git-master): The offending offending_part is: CREATE TABLE dupindexcols AS SELECT unique1 as id, stringu2::text as f1 FROM tenk1; CREATE INDEX dupindexcols_i ON dupindexcols (f1, id, f1 text_pattern_ops); ANALYZE dupindexcols; EXPLAIN (COSTS OFF) SELECT count(*) FROM dupindexcols WHERE f1 'WA' and id 1000 and f1 ~~ 'YX'; SELECT count(*) FROM dupindexcols WHERE f1 'WA' and id 1000 and f1 ~~ 'YX'; As I have no real idea of what ~~ is for an operator (I have looked it up as scalarltjoinsel), but I cannot find any semantics for it in the docs*... So I have no way of manually checking the expected result. *=The term ~~ is not exactly google-friendly and the docs site's search also returns empty... Anyone has any idea what to look after next? Svenne -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQI5BAEBCAAjBQJRwKTQHBpodHRwOi8vc3Zlbm5lLmRrL3BncC9wb2xpY3kACgkQ /zLSj+olL/KGHQ//d5WM2gi4yIaMCpaij1+PdCzBwyMwz0E3Ys4mlj1x7x22v8ty 6Gs6HZlofuuUdBkhDvRYYvbJ8asdi51KbjVMbrBkR49txiM00cWLtT74e9mFdTaA f5lOeUmfyqy7O9jlyFvKaC8hVRe7yQrbRK+b2sOBIq2TchVkvjT/AvvDcuPWCKD7 6FgHOtFF4Ae4yqC6foylfBQ59TpOYHBd+ohl38egtDr87tXSXcuIU1bNZeJfqcPM rs02h8L/kmWrx32TmYKLUw6PdluPEJ2WB5Gcsd8Va5p7ai3+rbqzzJGw9ZOFkPv4 9ruTqjTV0l7xhwuztHpSCTiyOoV0dIw13USTO7D/rAseqIUgb0mQE4cXVQW9GVT1 WjCqnaqRwdgYnvke7jtOfX2AeiZjluZfF1e2B2rVMfDan1A5PxjLmxpzH7fYs0bs RDEnOx9bcVUYH6xedVyYHbSmx7GXyzu0xfADsWgsYNclv91cZ5pz9+fwJv8ceNNz ckOcA4DlKeo1fw0idvbEUtVWf/tO8H6r0heNuMgR5M6qOtlbsLsdsJvOIUTzGn7W oxikvMAKbAX0Hl4arQSQE5Og7HcLsEB6YaMp0bpj55Y4prr/fhzJHKL4ioCI6NOH gLknu+IPDyJiNez4QqaqAITDtxo+68euv5uUE+O55o9ssBEWoTX0bz8trM8= =2IBo -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Git-master regression failure
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 18-06-2013 20:17, Kevin Grittner wrote: I was surprised to see that an index-test failed. It works for me. Could you paste or attach some detail? Gladly, if you tell me what would be relevant to attach :) I am brand new to the postgresql source code and hence have no real idea how to catch it.. Svenne -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQI5BAEBCAAjBQJRwKjRHBpodHRwOi8vc3Zlbm5lLmRrL3BncC9wb2xpY3kACgkQ /zLSj+olL/LIOQ/9E7PsAosZbQemCKeHj3/61kM1m3WMD9YtjZE4OdAJjEM+YCpE HkMheIpsIBo9rV1mH0mNt/rog0GZupGA9xWo+OsRPbQ3bd089ny++eSmGaJ4+nb3 cZiJpGizgQ+8xrMFLtkyt72WFUWashhb6IdokC1WeSrOWOcXGad+Sl48nxkODetR WJHuq0cdzLkivLSXBE3I5a7h28QWAwUHRlOUFNLaVTbVtPiv9Rx8YAnzYCvr3HC1 y+tdrHxONjoM4moLJH8qRQs6nMTCq+3mJgaKjW8FPCf5is5IMZLoACvjf0TOZm9o NnGv/R0TwLVjA/w44qMNJ4kXoX49IDX1IaYBvOraAjjWr+ggC2bmXwPZWqEHAZTG OCLD/t7DONW7uQEXEZSyur1N3CIZ231jl/ufYSefXaV81mtBVF6w7EVLi56UsC1A C626RY0r/87P9Y0avG1oh3ba6hOZhcv2R8GZZKkO7LNGUMSbIm90+FcV3bv/YSKi H5T3nYM0GWJ6kbqt+Mynqy9Q2N/33/rcpAL3ut0wIRkGALuIrb5zowi52y95+zAX ePpMFbM8I8nHuM0ouvcABHXtr6r5m6c1qwfJNbNQuSFz1T3s4/SY2W1IsinLSSKT /bqxP4qJxONDrIv9W4ZpIkxLYyfTHTuD3jP7GzndKcGHtHzMrTqxf1RGBs0= =hhm0 -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for fast gin cache performance improvement
No worries! I'll just wait until it's up again. Thanks Ian Kevin Grittner Tuesday, June 18, 2013 9:15 AM Oops -- we seem to have a problem with new community logins at themoment, which will hopefully be straightened out soon. You mightwant to wait a few days if you don't already have a login.--Kevin GrittnerEnterpriseDB: http://www.enterprisedb.comThe Enterprise PostgreSQL Company Kevin Grittner Tuesday, June 18, 2013 9:09 AM Ian Link i...@ilink.io wrote: This patch contains a performance improvement for the fast gin cache. Our test queries improve from about 0.9 ms to 0.030 ms. Impressive! Thanks for reading and considering this patch! Congratulations on your first PostgreSQL patch! To get it scheduled for review, please add it to this page: https://commitfest.postgresql.org/action/commitfest_view/open You will need to get a community login (if you don't already have one), but that is a quick and painless process. Choose an appropriate topic (like "Performance") and reference the message ID of the email to which you attached the patch. Don't worry about the fields for reviewers, committer, or date closed. Sorry for the administrative overhead, but without it things can fall through the cracks. You can find an overview of the review process with links to more detail here: http://wiki.postgresql.org/wiki/CommitFest Thanks for contributing! Ian Link Monday, June 17, 2013 9:42 PM This patch contains a performance improvement for the fast gin cache. As you may know, the performance of the fast gin cache decreases with its size. Currently, the size of the fast gin cache is tied to work_mem. The size of work_mem can often be quite high. The large size of work_mem is inappropriate for the fast gin cache size. Therefore, we created a separate cache size called gin_fast_limit. This global variable controls the size of the fast gin cache, independently of work_mem. Currently, the default gin_fast_limit is set to 128kB. However, that value could need tweaking. 64kB may work better, but it's hard to say with only my single machine to test on.On my machine, this patch results in a nice speed up. Our test queries improve from about 0.9 ms to 0.030 ms. Please feel free to use the test case yourself: it should be attached. I can look into additional test cases (tsvectors) if anyone is interested. In addition to the global limit, we have provided a per-index limit: fast_cache_size. This per-index limit begins at -1, which means that it is disabled. If the user does not specify a per-index limit, the index will simply use the global limit. I would like to thank Andrew Gierth for all his help on this patch. As this is my first patch he was extremely helpful. The idea for this performance improvement was entirely his. I just did the implementation. Thanks for reading and considering this patch! Ian Link
Re: [HACKERS] Git-master regression failure
Svenne Krap svenne.li...@krap.dk wrote: On 18-06-2013 20:17, Kevin Grittner wrote: I was surprised to see that an index-test failed. It works for me. Could you paste or attach some detail? Gladly, if you tell me what would be relevant to attach :) I am brand new to the postgresql source code and hence have no real idea how to catch it.. Apologies; I somehow missed the file attached to your initial post. That's the sort of thing I was looking for. Having reviewed that, the source code comments indicate it is for character-by-character (not collation order) comparison operators for character types. Perhaps your collation is having an impact regardless of that. Could you show us the values of your settings related to locale? -- Kevin Grittner 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
[HACKERS] review: Non-recursive processing of AND/OR lists
Hello
related to
https://commitfest.postgresql.org/action/patch_view?id=1130
http://www.postgresql.org/message-id/cabwtf4v9rsjibwe+87pk83mmm7acdrg7sz08rq-4qyme8jv...@mail.gmail.com
* motivation: remove recursive procession of AND/OR list (hangs with
10062 and more subexpressions)
* patch is short, clean and respect postgresql source code requirements
* patch was applied cleanly without warnings
* all regression tests was passed
* I successfully evaluated expression with 10 subexpressions
* there is no significant slowdown
possible improvements
a = (A_Expr*) list_nth(pending, 0);
a = (A_Expr*) linitial(pending);
not well comment
should be -- If the right branch is also an SAME condition, append it to the
+ /*
+* If the right branch is also an AND condition, append
it to the
+* pending list, to be processed later. This allows us
to walk even
+* bushy trees, not just left-deep trees.
+*/
+ if (IsA(a-rexpr, A_Expr) ((A_Expr*)a-rexpr)-kind
== root_kind)
+ {
+ pending = lappend(pending, a-rexpr);
+ }
+ else
+ {
+ expr = transformExprRecurse(pstate, a-rexpr);
+ expr = coerce_to_boolean(pstate, expr,
root_kind == AEXPR_AND ?
AND : OR);
+ exprs = lcons(expr, exprs);
+ }
I don't see any other issues, so after fixing comments this patch is
ready for commit
Regards
Pavel Stehule
--
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] Git-master regression failure
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 18-06-2013 20:48, Kevin Grittner wrote: Apologies; I somehow missed the file attached to your initial post. That's the sort of thing I was looking for. Aplogy accepted... :) Having reviewed that, the source code comments indicate it is for character-by-character (not collation order) comparison operators for character types. Perhaps your collation is having an impact regardless of that. Could you show us the values of your settings related to locale? I am not entirely sure what you mean by settings related to locale ... but here is the system locale output and the definition of my main instance (the 9.2 one I wrote about in the second test)...I don't know if/how to extract the same information from the temp instance made by make check. Btw. could you point a newbie to where you found the function (so that I know that for later)? If you need other data, please be more specific about how I get them :) # locale LANG=da_DK.UTF8 LC_CTYPE=da_DK.UTF8 LC_NUMERIC=da_DK.UTF8 LC_TIME=da_DK.UTF8 LC_COLLATE=da_DK.UTF8 LC_MONETARY=da_DK.UTF8 LC_MESSAGES=en_US.UTF8 LC_PAPER=da_DK.UTF8 LC_NAME=da_DK.UTF8 LC_ADDRESS=da_DK.UTF8 LC_TELEPHONE=da_DK.UTF8 LC_MEASUREMENT=da_DK.UTF8 LC_IDENTIFICATION=da_DK.UTF8 LC_ALL= (sk@[local]:5432) [sk] \l List of databases Name | Owner | Encoding | Collate | Ctype| Access privileges - -+--+--+++--- postgres| postgres | UTF8 | da_DK.UTF8 | da_DK.UTF8 | root| root | UTF8 | da_DK.UTF8 | da_DK.UTF8 | sk | sk | UTF8 | da_DK.UTF8 | da_DK.UTF8 | template0 | postgres | UTF8 | da_DK.UTF8 | da_DK.UTF8 | =c/postgres + | | ||| postgres=CTc/postgres template1 | postgres | UTF8 | da_DK.UTF8 | da_DK.UTF8 | =c/postgres + | | ||| postgres=CTc/postgres (5 rows) -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQI5BAEBCAAjBQJRwK87HBpodHRwOi8vc3Zlbm5lLmRrL3BncC9wb2xpY3kACgkQ /zLSj+olL/JVBQ//UQfke7jc2mcaTV9lvdU8b5B/SnGzsn8d2M2mDPYjcgjmuAmT pacRemmiVf8w6FTKlmz+A7faYVMuFSjJXi25OzvWu8on+zFtstdHWBbMNYI1CuBP itSqs+JVYF8FvkAa+iC9KilV1McRWCOxnlztuN7F29lzpnrcWsGmvhiYL3qVt3TT jyJNTdVIOFGta4f4Kzkq9ZqH6ry42UrXRFa1pFRxfY42zX7LV3BsJuEMQeHnRYjz fkcivuwJJHdn9l6GGZbF4+s227SFOO98GiPah10n5GKzy2saAMntos1p15l5Qy/c TaHE5cp0fttaQMbQEMWB3fY6r3NETxnsDyh6z7EaLV/WHhmzjrUGDp2gCgWbVs5C nvoFL9SkcfzZsuWvy21zmIHOvzAUMm9p57/i5REWqWc2ZH5giLKC71m7skiu/utF 9fmpNEh1U+DG3/VnJjcIyfNq4y3pmkNN063/aLKX21vvBHJ9/oqgL7czYRCBZH6L ufNL0ACZ/zeDTL16K58/gMhf7Si9jAZq7PcobIgSJHKo++7DevgUiSRmbxQ6jV6X ZXseeQJT7Dcf3yVNqNkPJApSiCbR4e0wbgbSPXXkJEjZpo+k3wo3gZI6fEULifdh i/hcLpq+nPrME2l9gcneJ8gNHpBOK4AyfBvfhxOIV35pCpYrBvk3toKPUEc= =bUcn -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Git-master regression failure
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 18-06-2013 21:04, Svenne Krap wrote: (sk@[local]:5432) [sk] \l List of databases Name | Owner | Encoding | Collate | Ctype| Access privileges - Arghh... crappy mailer... I have the information attached here instead... -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQI5BAEBCAAjBQJRwLACHBpodHRwOi8vc3Zlbm5lLmRrL3BncC9wb2xpY3kACgkQ /zLSj+olL/LJQw//avuPNPvG9sTy+itx9pmdRBHrdWCER4TXyKu15/0cphigMCHo mLNuLfcdwEbuppaqZgrEKP9GicppExmnGHGdgwn60CQiavki9HZpxuH+FX/+DRI6 IEM7Ml6PZw+8g+Uvh26vprgBhFw2B2xxx9uDEN5YEmpjJPqgxh8SXIs7Sy14FmEw uivU+YVLdoqLGUfSO3kng/ypTwqWC7vdycvZM6DtJdJLeZf+444kpX4TlVAncYtG LKRm7ym+9sUY5Tkq/cKNS/hM93fnrGGyD0bg6GtTHUKq0S0Cw79FcvCgWBNu0fOe QU6AtNIslvpxarECN2fkJ02S8pf0lnrDm8l6ZExO3BgY/Fo0wSPo7rhtdFEgPB6d uPy4YL9ezRfY7gkXWEgIX1zNA0h6vVcNi5OqbyC5WHenvI9mQPiwgwvbXTqCQmEF SKC6PoBOaJXaCajyXGVSjk+0IC/QYSMsqoWe1CcySFahfYFxKdHbkImQ02N+PB3x 9ABBuG2QUHTmh09MsCtr/l+McqlLVCPRngV5wYz5Z7guKu1lS6yzaAj6VHxVaw1A wCmVtJcCFNv/WhbarLOJOMsPjrQ5EXIOuM+k61nznooaqnBtChQTT1/ddzx+G3vU 0lX59b7tAM9+z2q+VCj57L6DgspCODMwHucv2MYkInS++ta0QV0U5ebzw/s= =/PiO -END PGP SIGNATURE- Expanded display is used automatically. Null display is NULL. Timing is on. List of databases Name | Owner | Encoding | Collate | Ctype| Access privileges -+--+--+++--- postgres| postgres | UTF8 | da_DK.UTF8 | da_DK.UTF8 | root| root | UTF8 | da_DK.UTF8 | da_DK.UTF8 | sk | sk | UTF8 | da_DK.UTF8 | da_DK.UTF8 | template0 | postgres | UTF8 | da_DK.UTF8 | da_DK.UTF8 | =c/postgres + | | ||| postgres=CTc/postgres template1 | postgres | UTF8 | da_DK.UTF8 | da_DK.UTF8 | =c/postgres + | | ||| postgres=CTc/postgres (5 rows) pgdb-locale.txt.sig Description: PGP signature -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Git-master regression failure
On Tue, Jun 18, 2013 at 11:20 AM, Svenne Krap svenne.li...@krap.dk wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 18-06-2013 18:40, Svenne Krap wrote: Any ideas what might have happened? After doing some more digging... My laptop (which runs PostgreSQL 9.2.4 on x86_64-pc-linux-gnu, compiled by x86_64-pc-linux-gnu-gcc (Gentoo 4.7.3 p1.0, pie-0.5.5) 4.7.3, 64-bit) also returns 99, if I - - run the CREATE TABLE tenk1 (from the git-master) - - load data from tenk.data (from git-master) - - run the offending part of the create_index.sql (also from git-master): But 9.2.4 does pass make check, and only fails if you reproduce those things manually? If so, I'm guessing that you have some language/locale settings that make check neutralizes in 9.2.4, but that neutralization is broken in HEAD. As I have no real idea of what ~~ is for an operator (I have looked it up as scalarltjoinsel), but I cannot find any semantics for it in the docs*... So I have no way of manually checking the expected result. Yes, it does seem to be entirely undocumented. Using: git grep '~~', I found the code comment character-by-character (not collation order) comparison operators for character types Anyway, if REL9_2_4 passes make check, but 073d7cb513f5de44530f fails, then you could use git bisect to find the exact commit that broke things. Cheers, Jeff
Re: [HACKERS] review: Non-recursive processing of AND/OR lists
Thanks for the review Pavel.
On Tue, Jun 18, 2013 at 3:01 PM, Pavel Stehule pavel.steh...@gmail.comwrote:
Hello
related to
https://commitfest.postgresql.org/action/patch_view?id=1130
http://www.postgresql.org/message-id/cabwtf4v9rsjibwe+87pk83mmm7acdrg7sz08rq-4qyme8jv...@mail.gmail.com
* motivation: remove recursive procession of AND/OR list (hangs with
10062 and more subexpressions)
* patch is short, clean and respect postgresql source code requirements
* patch was applied cleanly without warnings
* all regression tests was passed
* I successfully evaluated expression with 10 subexpressions
* there is no significant slowdown
possible improvements
a = (A_Expr*) list_nth(pending, 0);
a = (A_Expr*) linitial(pending);
not well comment
should be -- If the right branch is also an SAME condition, append it to
the
+ /*
+* If the right branch is also an AND condition,
append it to the
+* pending list, to be processed later. This
allows us to walk even
+* bushy trees, not just left-deep trees.
+*/
+ if (IsA(a-rexpr, A_Expr)
((A_Expr*)a-rexpr)-kind == root_kind)
+ {
+ pending = lappend(pending, a-rexpr);
+ }
+ else
+ {
+ expr = transformExprRecurse(pstate,
a-rexpr);
+ expr = coerce_to_boolean(pstate, expr,
root_kind == AEXPR_AND ?
AND : OR);
+ exprs = lcons(expr, exprs);
+ }
I don't see any other issues, so after fixing comments this patch is
ready for commit
Regards
Pavel Stehule
--
Gurjeet Singh
http://gurjeet.singh.im/
EnterpriseDB Inc.
Re: [HACKERS] Git-master regression failure
Jeff Janes escribió: On Tue, Jun 18, 2013 at 11:20 AM, Svenne Krap svenne.li...@krap.dk wrote: As I have no real idea of what ~~ is for an operator (I have looked it up as scalarltjoinsel), but I cannot find any semantics for it in the docs*... So I have no way of manually checking the expected result. Yes, it does seem to be entirely undocumented. Using: git grep '~~', I found the code comment character-by-character (not collation order) comparison operators for character types To look up an operator you can search in pg_operator.h (where you'd also see the DESCR() line nearby containing a description) or the pg_operator catalog. The pg_operator row contains a reference to the pg_proc entry that implements the operator. The pg_proc row, in turn, refers to a (typically) C-language function that implements the function. Normally looking at the function and surrounding code you can figure out what the operator is about. In this case you should probably be able to find the operator referenced in pg_amop as well, as part of the *_pattern_ops opclasses. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Git-master regression failure
Svenne Krap svenne.li...@krap.dk wrote: I have the information attached here instead... I find it suspicious that the test is using an index which sorts first by the f1 column, then later by f1 text_pattern_ops column. I'm not 100% sure whether the test is bad or you have found a bug, although I suspect the latter. The actual result should not depend on the index definition; the index should only affect performance and possibly the order of results where order is not specified. -- Kevin Grittner 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] Git-master regression failure
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 18-06-2013 21:14, Jeff Janes wrote: But 9.2.4 does pass make check, and only fails if you reproduce those things manually? No, I was lazy and used the (distribution-installed) 9.2 I have tried make check on REL_9_2_4 and that fails to (same sole failure)... If so, I'm guessing that you have some language/locale settings that make check neutralizes in 9.2.4, but that neutralization is broken in HEAD. Nope, just never ran make check on it... As I have no real idea of what ~~ is for an operator (I have looked it up as scalarltjoinsel), but I cannot find any semantics for it in the docs*... So I have no way of manually checking the expected result. Yes, it does seem to be entirely undocumented. Using: git grep '~~', I found the code comment character-by-character (not collation order) comparison operators for character types Anyway, if REL9_2_4 passes make check, but 073d7cb513f5de44530f fails, then you could use git bisect to find the exact commit that broke things. It does not.. I will dig futher and get back... Svenne -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQI5BAEBCAAjBQJRwLfnHBpodHRwOi8vc3Zlbm5lLmRrL3BncC9wb2xpY3kACgkQ /zLSj+olL/J4oA/7Blq1eQarny6VVu7UUgtN+CWvy3zoiE36AlE/vEvzb4aXWP4c 5Mq8z3vkHrjKCdaioGYLrkEE2PTom+pBV5cK+cT67TAN1J/OJErtRuKhVLk2Bd8I ry/gDgF8dvM0Sx03eN52ViCOjFUmksk+HXOSk8z0aPBdanbOdIQoSC25XJ2Yye8K DP+yY6h9+PxMomMzaz3aK0MechZaJyvbLpStjJEd/nuAiDIymzpzxJfyjpn/jKra 6GVdRfQCMkqiBy5E4YFgxDkGfDJLFtwMJPMw8OAe+Zhgp8YLPSms58bwtdmwcPYD NSfowZa/yaKpLP3k6J/2JVToa+JBPY/vYGlxdU+h15VmsDV8kxCWnRwxC/gYYg9l 7CqWPZskd9ZAonfb781JxoiVmoGbGzFCLE1wlPVazCZsAzQcvVTJD8yl/Ibv8PKv 7LjxuItm3iW8GAp93x7+MGmxKx/YKVu8lfAFTw2/X/G2MfoiJHmAUaklqdATerXl xarQOL0UbiRGQNYSBHjKVxZHQrgAShPS2O2cSw6MijaDsr4US+pcY+S0F0KmtnD/ mobI3vSFFsZxnaIUAL4ExQuyF0PgGm7Ce4jmQxsUx7Ph/ud6AZVLhKStYBcwChPH W+4Laq2fNB6/deH81x+x+j0QSk1oyvSVG+73/bmURWSP9WuhikYJ5kqbOdk= =siUv -END PGP SIGNATURE-
Re: [HACKERS] Git-master regression failure
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 18-06-2013 21:41, Svenne Krap wrote: I will dig futher and get back... The regression test was added in 9.2, the earliest interesting commit is d6d5f67b5b98b1685f9158e9d00a726afb2ae789, where Tom Lane changes the definition to the current. It still fails (which suggests that it has always and will always fail on my setup) I am happy to run whatever relevant tests you can dream up, but I am fresh out of ideas :) Svenne -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQI5BAEBCAAjBQJRwLo5HBpodHRwOi8vc3Zlbm5lLmRrL3BncC9wb2xpY3kACgkQ /zLSj+olL/JH5w/+PRU8Ghr56RuOcybp3S86Ig8sw4y56Xu8pb4RMHXNTJcL1zk0 fj0NRr+0sCAR9JXIS1mGHnNZuLpdppLklb0Zi02JeNbXelnY34QSiqgn3c9ikBIZ NTq63MVHj2bOC2YkpJIjCCZI7adjyOYm0Fosm453VV6lSh7PooNLUfqTlGA9lHKo lItndci7ei+JTCFr2G3zv2zshwNW87+nd9oAzf+n0Hz3djswrJWvdby56UtTb5S0 6ayYH14BnF0UU2C1Ko5/JSBgDyzT/kkPyn/YEplcHkO3yYKXgaYdanTyQEgYbL5I kSebe6eh271IY4W7wt+Bb5202HDOU6d3ikCJykE8G9BHw77exUbk27BJWipcUNL0 63IERJAleJ29yOqVZggZ0p3hf7H34lFqLKCTi7+p4mP1ZLlMlaMzbTGMdVJuc21A UyqQBY7nv9m7gIiDxXEQaoFbycn7Lb14xYgUOYIYGmnTblg4A5oUREXkMa08vWZg yq+oWOmusmbB3EET0jvQCQe9ves9J4Pa+5Axry2c4tbPQD2PH25FZGyAeCwJLon9 ZmMdFf3uNp91f+YW/BPDgXxi9A+fn1twI0ldMUYuX6MTopWvMrVHzpqQ3MJxPKLb RdWgKHWXZnu0ygw1F/AqpEgx2Y/SAyyuOId1cmamG8BXJsRZVgezqK+roww= =TzBa -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Git-master regression failure
Svenne Krap svenne.li...@krap.dk wrote: I am happy to run whatever relevant tests you can dream up, but I am fresh out of ideas :) psql regression begin; drop index dupindexcols_i; SELECT count(*) FROM dupindexcols WHERE f1 'WA' and id 1000 and f1 ~~ 'YX'; rollback; select f1 from dupindexcols where f1 like 'W%' ORDER BY f1; What are the results? -- Kevin Grittner 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] Git-master regression failure
On Tue, Jun 18, 2013 at 12:51 PM, Svenne Krap svenne.li...@krap.dk wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 18-06-2013 21:41, Svenne Krap wrote: I will dig futher and get back... The regression test was added in 9.2, the earliest interesting commit is d6d5f67b5b98b1685f9158e9d00a726afb2ae789, where Tom Lane changes the definition to the current. I get it back to e2c2c2e8b1df7dfdb01e7, where the ability to have one index with the same column twice appears. The problem is the f1 'WA' part of the query. In Danish, apparently 'AA' 'WA', so two more rows show up. SELECT f1 FROM dupindexcols WHERE f1 'WA' and id 1000 and f1 ~~ 'YX' except SELECT f1 FROM dupindexcols WHERE f1 ~~ 'WA' and id 1000 and f1 ~~ 'YX'; f1 AANAAA AAMAAA I don't know how important it is to the community for make check to pass under every possible LANG setting, or the best way to go about fixing it if it is important. Cheers, Jeff
Re: [HACKERS] Git-master regression failure
Jeff Janes jeff.ja...@gmail.com wrote: The problem is the f1 'WA' part of the query. In Danish, apparently 'AA' 'WA', so two more rows show up. Thanks -- I didn't have the right locale installed, and wasn't quite sure what package to install to get it. So, the test is bad, rather than there being a production bug. I don't know how important it is to the community for make check to pass under every possible LANG setting, or the best way to go about fixing it if it is important. We should probably tweak the test to not use a range which fails in any known locale. -- Kevin Grittner 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] GIN improvements part 1: additional information
On Mon, Jun 17, 2013 at 4:54 PM, Alexander Korotkov aekorot...@gmail.comwrote: On Fri, Jun 14, 2013 at 12:09 AM, Alexander Korotkov aekorot...@gmail.com wrote: Revised version of patch for additional information storage in GIN is attached. Changes are mostly bug fixes. New version of patch is attached with some more refactoring and bug fixes. Another revision with more bug fixes. -- With best regards, Alexander Korotkov. ginaddinfo.6.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] GIN improvements part2: fast scan
On Mon, Jun 17, 2013 at 5:09 PM, Heikki Linnakangas hlinnakan...@vmware.com
wrote:
On 17.06.2013 15:55, Alexander Korotkov wrote:
On Sat, Jun 15, 2013 at 2:55 AM, Alexander Korotkovaekorot...@gmail.com
**wrote:
attached patch implementing fast scan technique for GIN. This is second
patch of GIN improvements, see the 1st one here:
http://www.postgresql.org/**message-id/CAPpHfduxv-**
iL7aedwPW0W5fXrWGAKfxijWM63_**hzujacrxn...@mail.gmail.comhttp://www.postgresql.org/message-id/capphfduxv-il7aedwpw0w5fxrwgakfxijwm63_hzujacrxn...@mail.gmail.com
This patch allow to skip parts of posting trees when their scan is not
necessary. In particular, it solves frequent_term rare_term problem
of
FTS.
It introduces new interface method pre_consistent which behaves like
consistent, but:
1) allows false positives on input (check[])
2) allowed to return false positives
Some example: frequent_term rare_term becomes pretty fast.
create table test as (select to_tsvector('english', 'bbb') as v from
generate_series(1,100));
insert into test (select to_tsvector('english', 'ddd') from
generate_series(1,10));
create index test_idx on test using gin (v);
postgres=# explain analyze select * from test where v @@
to_tsquery('english', 'bbb ddd');
QUERY PLAN
--**--**
--**-
Bitmap Heap Scan on test (cost=942.75..7280.63 rows=5000 width=17)
(actual time=0.458..0.461 rows=10 loops=1)
Recheck Cond: (v @@ '''bbb'' ''ddd'''::tsquery)
- Bitmap Index Scan on test_idx (cost=0.00..941.50 rows=5000
width=0) (actual time=0.449..0.449 rows=10 loops=1)
Index Cond: (v @@ '''bbb'' ''ddd'''::tsquery)
Total runtime: 0.516 ms
(5 rows)
Attached version of patch has some refactoring and bug fixes.
Good timing, I just started looking at this.
I think you'll need to explain how this works. There are no docs, and
almost no comments.
Sorry for that. I'll post patch with docs and comments in couple days.
(and this shows how poorly I understand this, but) Why does this require
the additional information patch?
In principle, it doesn't require additional information patch. Same
optimization could be done without additional information. The reason why
it requires additional information patch is that otherwise I have to
implement and maintain 2 versions of fast scan: with and without
additional information.
What extra information do you store on-disk, in the additional information?
It depends on opclass. In regex patch it is part of graph associated with
trigram. In fulltext search it is word positions. In array similarity
search it could be length of array for better estimation of similarity (not
implemented yet). So it's anything which is stored with each ItemPointer
and is useful for consistent or index driven sorting (see patch #3).
The pre-consistent method is like the consistent method, but it allows
false positives. I think that's because during the scan, before having
scanned for all the keys, the gin AM doesn't yet know if the tuple contains
all of the keys. So it passes the keys it doesn't yet know about as 'true'
to pre-consistent.
Could that be generalized, to pass a tri-state instead of a boolean for
each key to the pre-consistent method? For each key, you would pass true,
false, or don't know. I think you could then also speed up queries like
!english bbb.
I would like to illustrate that on example. Imagine you have fulltext query
rare_term frequent_term. Frequent term has large posting tree while
rare term has only small posting list containing iptr1, iptr2 and iptr3. At
first we get iptr1 from posting list of rare term, then we would like to
check whether we have to scan part of frequent term posting tree where iptr
iptr1. So we call pre_consistent([false, true]), because we know that
rare term is not present for iptr iptr2. pre_consistent returns false. So
we can start scanning frequent term posting tree from iptr1. Similarly we
can skip lags between iptr1 and iptr2, iptr2 and iptr3, from iptr3 to
maximum possible pointer.
And yes, it could be generalized to tri-state instead of a boolean. I had
that idea first. But I found superiority of exact true quite narrow.
Let's see it on the example. If you have !term1 term2 there are few
cases:
1) term1 is rare, term2 is frequent = you can exclude only some entries
from posting tree of term2, performance benefit will be negligible
2) term1 is frequent, term2 is rare = you should actually scan term2
posting list and then check term1 posting tree like in example about
rare_term frequent_term, so there is no need of tri-state to handle
this situation
3) term1 is frequent, term2 is frequent = this case probably could be
optimized by tri-state. But in order to actully skip some parts of term2
posting tree you need item pointers
Re: [HACKERS] GIN improvements part 3: ordering in index
On Mon, Jun 17, 2013 at 10:27 PM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
On 17.06.2013 15:56, Alexander Korotkov wrote:
On Sat, Jun 15, 2013 at 3:02 AM, Alexander Korotkovaekorot...@gmail.com
**wrote:
This patch introduces new interface method of GIN which takes same
arguments as consistent but returns float8.
float8 gin_ordering(bool check[], StrategyNumber n, Datum query, int32
nkeys, Pointer extra_data[], bool *recheck, Datum queryKeys[], bool
nullFlags[], Datum addInfo[], bool addInfoIsNull[])
This patch implements gingettuple method which can return ordering data
using KNN infrastructure. Also it introduces operator for fts which
support ordering in GIN index. Some example:
postgres=# explain analyze select * from dblp_titles2 where tsvector @@
to_tsquery('english', 'statistics') order by tsvector
to_tsquery('english', 'statistics') limit 10;
QUERY
PLAN
--**--**
--**--**
-
Limit (cost=12.00..48.22 rows=10 width=136) (actual time=6.999..7.120
rows=10 loops=1)
- Index Scan using dblp_titles2_idx on dblp_titles2
(cost=12.00..43003.03 rows=11868 width=136) (actual time=6.996..7.115
rows=10 loops=1)
Index Cond: (tsvector @@ '''statist'''::tsquery)
Order By: (tsvector '''statist'''::tsquery)
Total runtime: 7.556 ms
(5 rows)
Attached version of patch has some refactoring and bug fixes.
Thanks. There are no docs changes and not many comments, that needs to be
fixed, but I think I understand how it works:
On the first call to gingettuple, the index is first scanned for all the
matches, which are collected in an array in memory. Then, the array is
sorted with qsort(), and the matches are returned one by one from the
sorted array.
Right.
That has some obvious limitations. First of all, you can run out of memory.
Yes, it is so. qsort should be replaced with tuplesort.
Secondly, is that really any faster than the plan you get without this
patch? Ie. scan all the matches with a bitmap index scan, and sort the
results on the rank function. If it is faster, why?
At, first it's obviously much faster when not both heap and index fit into
cache, because of IO. With patch you need only posting trees and posting
lists of requested entries. If query matching significant part of documents
then without patch you will need almost all the heap.
Also it's faster when both heap and index are in the cache. There are some
examples on DBLP paper titles (2.5M titles of 47 average length).
Without patch:
postgres=# explain analyze select id, s from dblp_titles2 where ts @@
to_tsquery('english', 'system') order by ts_rank(ts, to_tsquery('english',
'system')) desc limit 10;
QUERY
PLAN
Limit (cost=60634.15..60634.17 rows=10 width=134) (actual
time=200.204..200.205 rows=10 loops=1)
- Sort (cost=60634.15..61041.28 rows=162854 width=134) (actual
time=200.202..200.203 rows=10 loops=1)
Sort Key: (ts_rank(ts, '''system'''::tsquery))
Sort Method: top-N heapsort Memory: 28kB
- Bitmap Heap Scan on dblp_titles2 (cost=1758.12..57114.93
rows=162854 width=134) (actual time=33.592..158.006 rows=168308 loops=1)
Recheck Cond: (ts @@ '''system'''::tsquery)
- Bitmap Index Scan on dblp_titles2_idx
(cost=0.00..1717.41 rows=162854 width=0) (actual time=27.327..27.327
rows=168308 loops=1)
Index Cond: (ts @@ '''system'''::tsquery)
Total runtime: 200.610 ms
(9 rows)
With patch:
postgres=# explain analyze select id, s from dblp_titles2 where ts @@
to_tsquery('english', 'system') order by ts to_tsquery('english',
'system') limit 10;
QUERY
PLAN
-
Limit (cost=12.00..43.05 rows=10 width=136) (actual time=39.585..39.597
rows=10 loops=1)
- Index Scan using dblp_titles2_idx on dblp_titles2
(cost=12.00..493681.61 rows=159005 width=136) (actual time=39.584..39.593
rows=10 loops=1)
Index Cond: (ts @@ '''system'''::tsquery)
Order By: (ts '''system'''::tsquery)
Total runtime: 40.115 ms
(5 rows)
Without patch:
postgres=# explain analyze select id, s, ts_rank(ts, to_tsquery('english',
'statistics')) from dblp_titles2 where ts @@ to_tsquery('english',
'statistics') order by ts_rank(ts, to_tsquery('english', 'statistics'))
desc limit 10;
QUERY PLAN
Re: [HACKERS] SET work_mem = '1TB';
On Wed, Jun 19, 2013 at 2:40 AM, Simon Riggs si...@2ndquadrant.com wrote: On 18 June 2013 17:10, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Jun 18, 2013 at 1:06 PM, Jeff Janes jeff.ja...@gmail.com wrote: On Tuesday, May 21, 2013, Simon Riggs wrote: I worked up a small patch to support Terabyte setting for memory. Which is OK, but it only works for 1TB, not for 2TB or above. I've incorporated my review into a new version, attached. Added TB to the docs, added the macro KB_PER_TB, and made show to print 1TB rather than 1024GB. Looks good to me. But I found you forgot to change postgresql.conf.sample, so I changed it and attached the updated version of the patch. Barring any objection to this patch and if no one picks up this, I will commit this. In truth, I hadn't realised somebody had added this to the CF. It was meant to be an exploration and demonstration that further work was/is required rather than a production quality submission. AFAICS it is still limited to '1 TB' only... Yes. Thank you both for adding to this patch. Since you've done that, it seems churlish of me to interrupt that commit. I was thinking that this is the infrastructure patch for your future proposal, i.e., support higher values of TBs. But if it interferes with your future proposal, of course I'm okay to drop this patch. Thought? Regards, -- Fujii Masao -- 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] Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls
Thanks for the reviews. I've attached a revised patch that has the lexer refactoring Alvaro mentions (arbitarily using a goto rather than a bool flag) and uses Jeff's idea of just storing the index of the last non-null value (if there is one) in the window function's context (and not the Datum itself). However, Robert's right that SELECT ... ORDER BY respect NULLS LAST will now fail. An earlier iteration of the patch had RESPECT and IGNORE as reserved, but that would have broken tables with columns called respect (etc.), which the current version allows. Is this backwards incompatibility acceptable? If not, maybe I should try doing a two-token lookahead in the token-aggregating code between the lexer and the parser (i.e. make a RESPECT_NULLS token out of a sequence of RESPECT NULLS OVER tokens, remembering to replace the OVER token)? Or what about adding an %expect statement to the Bison grammar, confirming that the shift / reduce conflicts caused by using the RESPECT, IGNORE NULL_P tokens the in out_clause rule are OK? Thanks - Nick lead-lag-ignore-nulls.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Memory leak in PL/pgSQL function which CREATE/SELECT/DROP a temporary table
From: Heikki Linnakangas hlinnakan...@vmware.com On 18.06.2013 15:48, Heikki Linnakangas wrote: Hmm. I could repeat this, and it seems that the catcache for pg_statistic accumulates negative cache entries. Those slowly take up the memory. Digging a bit deeper, this is a rather common problem with negative catcache entries. In general, nothing stops you from polluting the cache with as many negative cache entries as you like. Just do select * from table_that_doesnt_exist for as many non-existent table names as you want, for example. Those entries are useful at least in theory; they speed up throwing the error the next time you try to query the same non-existent table. Really? Would the catcache be polluted with entries for nonexistent tables? I'm surprised at this. I don't think it is necessary to speed up the query that fails with nonexistent tables, because such queries should be eliminated during application development. But there is a crucial difference in this case; the system created a negative cache entry for the pg_statistic row of the table, but once the relation is dropped, the cache entry keyed on the relation's OID, is totally useless. It should be removed. We have this problem with a few other catcaches too, which have what is effectively a foreign key relationship with another catalog. For example, the RELNAMENSP catcache is keyed on pg_class.relname, pg_class.relnamespace, yet any negative entries are not cleaned up when the schema is dropped. If you execute this repeatedly in a session: CREATE SCHEMA foo; SELECT * from foo.invalid; -- throws an error DROP SCHEMA foo; it will leak similarly to the original test case, but this time the leak is into the RELNAMENSP catcache. To fix that, I think we'll need to teach the catalog cache about the relationships between the caches. Thanks for your concise explanation. Do you think it is difficult to fix that bug? That sounds so to me... though I don't know the design of catcaches yet. Could you tell me the conditions where this bug occurs and how to avoid it? I thought of the following: [Condition] 1. Create and drop the same table repeatedly on the same session. Whether the table is a temporary table is irrelevant. 2. Do SELECT against the table. INSERT/DELETE/UPDATE won't cause the catcache leak. 3. Whether the processing happens in a PL/pgSQL function is irrelevant. The leak occurs even when you do not use PL/pgSQL. [Wordaround] Use CREATE TABLE IF NOT EXISTS and TRUNCATE (or ON COMMIT DROP in case of temporary tables) to avoid repeated creation/deletion of the same table. Regards MauMau -- 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] LEFT JOIN LATERAL can remove rows from LHS
On 06/18/2013 01:52 AM, Jeremy Evans wrote: Maybe I am misunderstanding how LATERAL is supposed to work, but my expectation is that doing a LEFT JOIN should not remove rows from the LHS. I would expect all of the following select queries would return a single row, but that isn't the case: CREATE TABLE i (n integer); CREATE TABLE j (n integer); INSERT INTO i VALUES (10); SELECT * FROM i LEFT JOIN LATERAL (SELECT * FROM j) j ON true; n | n +--- 10 | (1 row) SELECT * FROM i LEFT JOIN LATERAL (SELECT * FROM j WHERE (i.n = j.n)) j ON true; n | n ---+--- (0 rows) INSERT INTO j VALUES (10); SELECT * FROM i LEFT JOIN LATERAL (SELECT * FROM j WHERE (i.n = j.n)) j ON true; n | n + 10 | 10 (1 row) SELECT * FROM i LEFT JOIN LATERAL (SELECT * FROM j WHERE (i.n = j.n)) j ON false; n | n ---+--- (0 rows) This is a bug. If you block the optimizer from rearranging the lateral join condition, it gives the correct answer: No blocking: SELECT * FROM i LEFT JOIN LATERAL (SELECT * FROM j WHERE (i.n = j.n)) j ON true; n | n ---+--- (0 rows) QUERY PLAN - Nested Loop Left Join (cost=0.00..65.01 rows=12 width=8) (actual time=0.027..0.027 rows=0 loops=1) Filter: (i.n = j.n) Rows Removed by Filter: 1 - Seq Scan on i (cost=0.00..1.01 rows=1 width=4) (actual time=0.013..0.015 rows=1 loops=1) - Seq Scan on j (cost=0.00..34.00 rows=2400 width=4) (actual time=0.001..0.001 rows=0 loops=1) Total runtime: 0.084 ms (6 rows) Blocking: SELECT * FROM i LEFT JOIN LATERAL (SELECT * FROM j WHERE (i.n = j.n) OFFSET 0) j ON true; n | n +--- 10 | (1 row) QUERY PLAN - Nested Loop Left Join (cost=0.00..41.25 rows=12 width=8) (actual time=0.014..0.015 rows=1 loops=1) - Seq Scan on i (cost=0.00..1.01 rows=1 width=4) (actual time=0.006..0.007 rows=1 loops=1) - Seq Scan on j (cost=0.00..40.00 rows=12 width=4) (actual time=0.001..0.001 rows=0 loops=1) Filter: (i.n = n) Total runtime: 0.057 ms (5 rows) Vik -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Memory leak in PL/pgSQL function which CREATE/SELECT/DROP a temporary table
On Tue, Jun 18, 2013 at 3:40 PM, MauMau maumau...@gmail.com wrote: From: Heikki Linnakangas hlinnakan...@vmware.com On 18.06.2013 15:48, Heikki Linnakangas wrote: Hmm. I could repeat this, and it seems that the catcache for pg_statistic accumulates negative cache entries. Those slowly take up the memory. Digging a bit deeper, this is a rather common problem with negative catcache entries. In general, nothing stops you from polluting the cache with as many negative cache entries as you like. Just do select * from table_that_doesnt_exist for as many non-existent table names as you want, for example. Those entries are useful at least in theory; they speed up throwing the error the next time you try to query the same non-existent table. Really? Would the catcache be polluted with entries for nonexistent tables? I'm surprised at this. I don't think it is necessary to speed up the query that fails with nonexistent tables, because such queries should be eliminated during application development. I was thinking the same thing, optimizing for failure is nice if there are no tradeoffs, but not so nice if it leaks memory. But apparently the negative cache was added for real reasons, not just theory. See discussion from when it was added: http://www.postgresql.org/message-id/19585.1012350...@sss.pgh.pa.us Cheers, Jeff
Re: [HACKERS] Support for REINDEX CONCURRENTLY
On Wed, Jun 19, 2013 at 12:36 AM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Jun 18, 2013 at 10:53 AM, Michael Paquier michael.paqu...@gmail.com wrote: An updated patch for the toast part is attached. On Tue, Jun 18, 2013 at 3:26 AM, Fujii Masao masao.fu...@gmail.com wrote: Here are the review comments of the removal_of_reltoastidxid patch. I've not completed the review yet, but I'd like to post the current comments before going to bed ;) *** a/src/backend/catalog/system_views.sql -pg_stat_get_blocks_fetched(X.oid) - -pg_stat_get_blocks_hit(X.oid) AS tidx_blks_read, -pg_stat_get_blocks_hit(X.oid) AS tidx_blks_hit +pg_stat_get_blocks_fetched(X.indrelid) - +pg_stat_get_blocks_hit(X.indrelid) AS tidx_blks_read, +pg_stat_get_blocks_hit(X.indrelid) AS tidx_blks_hit ISTM that X.indrelid indicates the TOAST table not the TOAST index. Shouldn't we use X.indexrelid instead of X.indrelid? Indeed good catch! We need in this case the statistics on the index and here I used the table OID. Btw, I also noticed that as multiple indexes may be involved for a given toast relation, it makes sense to actually calculate tidx_blks_read and tidx_blks_hit as the sum of all stats of the indexes. Yep. You seem to need to change X.indexrelid to X.indrelid in GROUP clause. Otherwise, you may get two rows of the same table from pg_statio_all_tables. I changed it a little bit in a different way in my latest patch by adding a sum on all the indexes when getting tidx_blks stats. doc/src/sgml/diskusage.sgml There will be one index on the acronymTOAST/ table, if present. + table (see xref linkend=storage-toast). There will be one valid index + on the acronymTOAST/ table, if present. There also might be indexes When I used gdb and tracked the code path of concurrent reindex patch, I found it's possible that more than one *valid* toast indexes appear. Those multiple valid toast indexes are viewable, for example, from pg_indexes. I'm not sure whether this is the bug of concurrent reindex patch. But if it's not, you seem to need to change the above description again. Not sure about that. The latest code is made such as only one valid index is present on the toast relation at the same time. *** a/src/bin/pg_dump/pg_dump.c + SELECT c.reltoastrelid, t.indexrelid FROM pg_catalog.pg_class c LEFT JOIN - pg_catalog.pg_class t ON (c.reltoastrelid = t.oid) - WHERE c.oid = '%u'::pg_catalog.oid;, + pg_catalog.pg_index t ON (c.reltoastrelid = t.indrelid) + WHERE c.oid = '%u'::pg_catalog.oid AND t.indisvalid + LIMIT 1, Is there the case where TOAST table has more than one *valid* indexes? I just rechecked the patch and is answer is no. The concurrent index is set as valid inside the same transaction as swap. So only the backend performing the swap will be able to see two valid toast indexes at the same time. According to my quick gdb testing, this seems not to be true Well, I have to disagree. I am not able to reproduce it. Which version did you use? Here is what I get with the latest version of REINDEX CONCURRENTLY patch... I checked with the following process: 1) Create this table: CREATE TABLE aa (a int, b text); ALTER TABLE aa ALTER COLUMN b SET STORAGE EXTERNAL; 2) Create session 1 and take a breakpoint on ReindexRelationConcurrently:indexcmds.c 3) Launch REINDEX TABLE CONCURRENTLY aa 4) With a 2nd session, Go through all the phases of the process and scanned the validity of toast indexes with the following ioltas=# select pg_class.relname, indisvalid, indisready from pg_class, pg_index where pg_class.reltoastrelid = pg_index.indrelid and pg_class.relname = 'aa'; relname | indisvalid | indisready -++ aa | t | t aa | f | t (2 rows) When scanning all the phases with the 2nd psql session (the concurrent index creation, build, validation, swap, and drop of the concurrent index), I saw at no moment that indisvalid was set at true for the two indexes at the same time. indisready was of course changed to prepare the concurrent index to be ready for inserts, but that was all and this is part of the process. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pgbench --throttle (submission 7 - with lag measurement)
I'm still getting the same sort of pauses waiting for input with your v11. This is a pretty frustrating problem; I've spent about two days so far trying to narrow down how it happens. I've attached the test program I'm using. It seems related to my picking a throttled rate that's close to (but below) the maximum possible on your system. I'm using 10,000 on a system that can do about 16,000 TPS when running pgbench in debug mode. This problem is 100% reproducible here; happens every time. This is a laptop running Mac OS X. It's possible the problem is specific to that platform. I'm doing all my tests with the database itself setup for development, with debug and assertions on. The lag spikes seem smaller without assertions on, but they are still there. Here's a sample: transaction type: SELECT only scaling factor: 10 query mode: simple number of clients: 25 number of threads: 1 duration: 30 s number of transactions actually processed: 301921 average transaction lag: 1.133 ms (max 137.683 ms) tps = 10011.527543 (including connections establishing) tps = 10027.834189 (excluding connections establishing) And those slow ones are all at the end of the latency log file, as shown in column 3 here: 22 11953 3369 0 1371578126 954881 23 11926 3370 0 1371578126 954918 3 12238 30310 0 1371578126 984634 7 12205 30350 0 1371578126 984742 8 12207 30359 0 1371578126 984792 11 12176 30325 0 1371578126 984837 13 12074 30292 0 1371578126 984882 0 12288 175452 0 1371578127 126340 9 12194 171948 0 1371578127 126421 12 12139 171915 0 1371578127 126466 24 11876 175657 0 1371578127 126507 Note that there are two long pauses here, which happens maybe half of the time. There's a 27 ms pause and then a 145 ms one. The exact sequence for when the pauses show up is: -All remaining clients have sent their SELECT statement and are waiting for a response. They are not sleeping, they're waiting for the server to return a query result. -A client receives part of the data it wants, but there is still data left. This is the path through pgbench.c where the if (PQisBusy(st-con)) test is true after receiving some information. I hacked up some logging that distinguishes those as a client %d partial receive to make this visible. -A select() call is made with no timeout, so it can only be satisfied by more data being returned. -Around ~100ms (can be less, can be more) goes by before that select() returns more data to the client, where normally it would happen in ~2ms. You were concerned about a possible bug in the timeout code. If you hack up the select statement to show some state information, the setup for the pauses at the end always looks like this: calling select min_usec=9223372036854775807 sleeping=0 When no one is sleeping, the timeout becomes infinite, so only returning data will break it. This is intended behavior though. This exact same sequence and select() call parameters happen in pgbench code every time at the end of a run. But clients without the throttling patch applied never seem to get into the state where they pause for so long, waiting for one of the active clients to receive the next bit of result. I don't think the st-listen related code has anything to do with this either. That flag is only used to track when clients have completed sending their first query over to the server. Once reaching that point once, afterward they should be listening for results each time they exit the doCustom() code. st-listen goes to 1 very soon after startup and then it stays there, and that logic seems fine too. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com #!/bin/bash make make install rm pgbench_log* nohup.out #nohup pgbench -c 25 -T 30 -l -d -f select.sql -s 10 pgbench nohup pgbench -c 25 -T 30 -l -d -S -R 1 pgbench tail -n 50 pgbench_log* -- 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] event trigger API documentation?
On Mon, 2013-05-06 at 17:17 +0200, Dimitri Fontaine wrote: Peter Eisentraut pete...@gmx.net writes: At this point, all that is appropriate is some documentation of the C API. If the contrib example you have in mind is short enough, it might as well become part of the example in the documentation. Please find attached a patch against the documentation, containing a full code example of what I had in mind. The contrib would only be useful to include if we want to ship something usable. As you might want to tinker with the code in the docs patch and easily check that it still runs, I include another patch with the new contrib module. I don't expect that to get commited, of course, but I had to do it to check the code so I'd better just share it, right? Looks pretty good, but the description of the parsetree field was obviously copied from somewhere else. I would fix it myself, but I don't know what kind of assurances we want to offer about what's in that field. -- 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 for removng unused targets
Hi Alexander, Thank you for the check! I marked the patch ready for committer. Best regards, Etsuro Fujita From: Alexander Korotkov [mailto:aekorot...@gmail.com] Sent: Wednesday, June 19, 2013 1:26 AM To: Etsuro Fujita Cc: Tom Lane; pgsql-hackers Subject: Re: [HACKERS] Patch for removng unused targets Hi Etsuro! On Tue, Jun 18, 2013 at 4:15 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: Hi Alexander, I wrote: From: Tom Lane [mailto:t...@sss.pgh.pa.us] resjunk means that the target is not supposed to be output by the query. Since it's there at all, it's presumably referenced by ORDER BY or GROUP BY or DISTINCT ON, but the meaning of the flag doesn't depend on that. What you would need to do is verify that the target is resjunk and not used in any clause besides ORDER BY. I have not read your patch, but I rather imagine that what you've got now is that the parser checks this and sets the new flag for consumption far downstream. Why not just make the same check in the planner? I've created a patch using this approach. I've rebased the above patch against the latest head. Could you review the patch? If you have no objection, I'd like to mark the patch ready for committer. Sorry, I've had a cleanup of the patch. Please find attached the patch. I've checked the attached patch. It looks good for me. No objection to mark it ready for committer. -- With best regards, Alexander Korotkov.
Re: [HACKERS] Vacuum, Freeze and Analyze: the big picture
On Jun 3, 2013, at 6:45 PM, Craig Ringer cr...@2ndquadrant.com wrote: On 06/04/2013 05:27 AM, Peter Geoghegan wrote: On Mon, Jun 3, 2013 at 5:03 AM, Craig Ringer cr...@2ndquadrant.com wrote: I've seen cases on Stack Overflow and elsewhere in which disk merge sorts perform vastly better than in-memory quicksort, so the user benefited from greatly *lowering* work_mem. I've heard of that happening on Oracle, when the external sort is capable of taking advantage of I/O parallelism, but I have a pretty hard time believing that it could happen with Postgres under any circumstances. IIRC it's usually occurred with very expensive comparison operations. I'll see if I can find one of the SO cases. FWIW, I've definitely seen this behavior in the past, on really old versions (certainly pre-9, possibly pre-8). IIRC there's some kind of compression or something used with on-disk sorts. If that's correct then I think what's happening is that the on-disk sort that fits into cache is actually using less memory than quicksort. Or perhaps it was just a matter of memory locality within each tape. It's been too long since I looked at it. :( -- 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] How do we track backpatches?
On Tue, 2013-06-18 at 12:32 +0200, Magnus Hagander wrote: The CF app was and is specifically for dealing with CFs. Having it deal with backpatches makes it, well, a bugtracker. It's not meant to be that. If we want a bugtracker, then it has very different requirements. It's not in evidence that the requirements are different. The CF app is basically a list of lists of patches with date information and associated person's names. Tracking backpatch candidates doesn't sound that much different. (That said, I'm not convinced backpatches need any tracking at all, but if they did, I think the CF app would be just fine.) Having an always-open CF would defeat the workflow. I'd imagine having a CF entry per release, so after a set of minor releases, the CF is closed. But since those patches are typically going into HEAD as well, why not just a commitfest *topic* for it, on whatever commitfest happens to be the open one? Then it'll get processed within the existing workflow. But then how do you represent that the actual commit fest is closed, and how do you, well, actually track the patches that need to be backpatched? -- 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] Bugfix and new feature for PGXS
On Tue, 2013-06-18 at 15:52 +0200, Cédric Villemain wrote: This allows for example to install hstore header and be able to include them in another extension like that: # include contrib/hstore/hstore.h That's not going to work. hstore's header file is included as #include hstore.h (cf. hstore source code). Having it included under different paths in different contexts will be a mess. -- 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] Remove useless USE_PGXS support in contrib
On Fri, 2013-06-14 at 09:32 -0400, Andrew Dunstan wrote: I do think we need to make sure that we have at least buildfarm coverage of pgxs module building and testing. I have some coverage of a few extensions I have written, which exercise that, so maybe that will suffice. If not, maybe we need to have one module that only builds via pgxs and is build after an install (i.e. not via the standard contrib build). The way to test pgxs is to build actual pgxs modules, and possibly some unit tests made for the purpose. Testing with fake hybrid modules that will never actually appear that way in practice isn't going to satisfy anyone. -- 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] Remove useless USE_PGXS support in contrib
On Sun, 2013-06-16 at 18:20 +0200, Cédric Villemain wrote: Also I suggest to remove the need to set REGRESS at all, and default to all sql files in REGRESSDIR/sql (if REGRESSDIR is set) I'm not so sure about that. I have some extensions where the list of tests is composed at build time depending on the target version (e.g., supports extensions, inline, event triggers, etc.). In a different world, we'd have skip functionality for that sort of thing, but for now building up the list of tests seems best. -- 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] Remove useless USE_PGXS support in contrib
On Mon, 2013-06-17 at 19:00 +0200, Cédric Villemain wrote: My only grief is to loose the perfect regression tests for PGXS those contribs are. I think they are neither perfect nor regression tests. If we want tests, let's write tests. -- 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] Remove useless USE_PGXS support in contrib
On Mon, 2013-06-17 at 11:41 +0200, Dimitri Fontaine wrote: I agree that having both cases (sections) in the Makefile is a bad idea. Still, why should we keep the in-tree build instructions? Would it be possible instead to instruct PGXN to work with a non installed server source tree? And how much do we need that really? We could do something like PG_CONFIG = fake_intree_pg_config PGXS := $(shell $(PG_CONFIG) --pgxs) include $(PGXS) where fake_intree_pg_config is a purpose-built shell script that points to the right places inside the source tree. -- 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 to add support of IF NOT EXISTS to others CREATE statements
On Mon, Jun 17, 2013 at 12:36 AM, Robins Tharakan thara...@gmail.comwrote: Hi, Did some basic checks on this patch. List-wise feedback below. [...] Dear Robins, Thanks for your review. I attach your considerations to Commit Fest [1]. Regards, [1] https://commitfest.postgresql.org/action/patch_view?id=1133 -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
