Re: [HACKERS] Row-security on updatable s.b. views
On 02/06/2014 10:19 PM, Craig Ringer wrote: On 02/06/2014 12:43 PM, Craig Ringer wrote: 1. Try (again) to do row-security in the rewriter. This was previously impossible because of the definition of row-security behaviour around inheritance, but with the simplified inheritance model now proposed I think it's possible. Thanks to the simplified requirements for inheritance, this turns out to be fairly easy. There's a version rewritten to use the rewriter in the tag: rls-9.4-upd-sb-views-v6 on https://github.com/ringerc/postgres.git ... which was totally wrong, and I blame lack of sleep for it ever getting pushed. I didn't understand the rewriter as well as I thought. v7 applies row-security quals in fireRIRrules . It handles recursion correctly now, and works fine for both target and non-target relations. I've cleaned out most of the cruft from the previous optimizer based approach too. I haven't figured out how to pass the plan invalidation information (so plans are invalidated properly when they depend on row security quals) down into the planner yet, that's next. COPY still just ERROR's if you try to copy to/from a rel with row-security quals, but again, just a matter of getting it done, I have KaiGai's patch to work from. Regression tests fail, including a segfault in the executor. Cause as yet unknown, but it takes a hairy view+rowsecrule combo to trigger it. New tag: rls-9.4-upd-sb-views-v6 The regression test expected file needs adjustment to match the new inheritance rules, and to cover a couple of new tests I've added. Still true. Need to cherry-pick the docs patch back on top of the patch tree now things are settling. Still true. -- Craig Ringer 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] jsonb and nested hstore
On 02/11/2014 01:16 AM, Merlin Moncure wrote: On Mon, Feb 10, 2014 at 5:52 PM, Andres Freund and...@2ndquadrant.com wrote: It works in enough cases atm that it's worthwile trying to keep it working. Sure, it could be better, but it's what we have right now. Atm it's e.g. the only realistic way to copy larger amounts of bytea between servers without copying the entire cluster. That's the thing -- it might work today, but what about tomorrow? We'd be sending the wrong signals. People start building processes around all of this and now we've painted ourselves into a box. Better in my mind to simply educate users that this practice is dangerous and unsupported, as we used to do. I guess until now. It seems completely odd to me that we're attaching a case to the jsonb type, in the wrong way -- something that we've never attached to any other type before. For example, why didn't we attach a version code to the json type send function? JSON is supposed to be a *standard* way of encoding data in strings. If the ever changes, it will not be JSON type anymore. Cheers -- Hannu Krosing PostgreSQL Consultant Performance, Scalability and High Availability 2ndQuadrant Nordic OÜ -- 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] Row-security on updatable s.b. views
On 2014-02-11 09:36, Craig Ringer wrote: On 02/06/2014 10:19 PM, Craig Ringer wrote: On 02/06/2014 12:43 PM, Craig Ringer wrote: 1. Try (again) to do row-security in the rewriter. This was previously impossible because of the definition of row-security behaviour around inheritance, but with the simplified inheritance model now proposed I think it's possible. Thanks to the simplified requirements for inheritance, this turns out to be fairly easy. There's a version rewritten to use the rewriter in the tag: rls-9.4-upd-sb-views-v6 on https://github.com/ringerc/postgres.git ... which was totally wrong, and I blame lack of sleep for it ever getting pushed. I didn't understand the rewriter as well as I thought. v7 applies row-security quals in fireRIRrules . New tag: rls-9.4-upd-sb-views-v6 Hi Craig, This looks to be the same v6 version as the initial rewriter version. https://github.com/ringerc/postgres/commits/rls-9.4-upd-sb-views-v6 regards, Yeb -- 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] Row-security on updatable s.b. views
On 02/11/2014 06:05 PM, Yeb Havinga wrote: On 2014-02-11 09:36, Craig Ringer wrote: On 02/06/2014 10:19 PM, Craig Ringer wrote: On 02/06/2014 12:43 PM, Craig Ringer wrote: 1. Try (again) to do row-security in the rewriter. This was previously impossible because of the definition of row-security behaviour around inheritance, but with the simplified inheritance model now proposed I think it's possible. Thanks to the simplified requirements for inheritance, this turns out to be fairly easy. There's a version rewritten to use the rewriter in the tag: rls-9.4-upd-sb-views-v6 on https://github.com/ringerc/postgres.git ... which was totally wrong, and I blame lack of sleep for it ever getting pushed. I didn't understand the rewriter as well as I thought. v7 applies row-security quals in fireRIRrules . New tag: rls-9.4-upd-sb-views-v6 Hi Craig, This looks to be the same v6 version as the initial rewriter version. https://github.com/ringerc/postgres/commits/rls-9.4-upd-sb-views-v6 Whoops, wrong paste. rls-9.4-upd-sb-views-v7 -- Craig Ringer 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] Row-security on updatable s.b. views
On 2014-02-11 12:09, Craig Ringer wrote:
On 02/11/2014 06:05 PM, Yeb Havinga wrote:
On 2014-02-11 09:36, Craig Ringer wrote:
On 02/06/2014 10:19 PM, Craig Ringer wrote:
On 02/06/2014 12:43 PM, Craig Ringer wrote:
1. Try (again) to do row-security in the rewriter. This was previously
impossible because of the definition of row-security behaviour around
inheritance, but with the simplified inheritance model now proposed I
think it's possible.
Thanks to the simplified requirements for inheritance, this turns out to
be fairly easy. There's a version rewritten to use the rewriter in
the tag:
rls-9.4-upd-sb-views-v6
on https://github.com/ringerc/postgres.git
... which was totally wrong, and I blame lack of sleep for it ever
getting pushed. I didn't understand the rewriter as well as I thought.
v7 applies row-security quals in fireRIRrules .
New tag:
rls-9.4-upd-sb-views-v6
Hi Craig,
This looks to be the same v6 version as the initial rewriter version.
https://github.com/ringerc/postgres/commits/rls-9.4-upd-sb-views-v6
Whoops, wrong paste.
rls-9.4-upd-sb-views-v7
Hi Craig,
I compared output of psql -ef of the minirim.sql script posted earlier
in http://www.postgresql.org/message-id/52f54927.1040...@gmail.com
between v4 and v7.
Not everything is ok.
Seq Scan on patient (cost=0.00..29589.31 rows=495 width=52)
Filter: (SubPlan 1)
SubPlan 1
@@ -555,7 +592,7 @@
- Materialize (cost=26.39..570.62 rows=1014 width=4)
- Subquery Scan on act (cost=26.39..565.55
rows=1014 width=4)
- Nested Loop Semi Join
(cost=26.39..555.41 rows=1014 width=108)
- Join Filter: (((part.act = act_1.id)
AND (emp_2.pgname = (current_user())::text)) OR (NOT
((act_1.confidentialitycode)::text[] @ '{s}'::text[])))
+ Join Filter: (((part.act = act_1.id)
AND (emp_2.pgname = (current_user())::text)) OR (NOT
((act_1.effectivetime)::text[] @ '{s}'::text[])))
- Append (cost=0.00..31.19
rows=1019 width=108)
- Seq Scan on act act_1
(cost=0.00..1.59 rows=59 width=108)
@@ -587,12 +624,8 @@
FROM patient, person, organization
WHERE patient.player = person.id
AND patient.scoper = organization.id;
- id | vipcode | name | birthtime | name
-+-+--+-+
- 10 | | John Doe | 1963-04-01 00:00:00 | Community Health and
Hospitals
- 16 | | John Doe | 1963-04-01 00:00:00 | Community Mental
Health Clinic
-(2 rows)
-
+psql:/home/m/minirim2.sql:409: ERROR: attribute 6 has wrong type
+DETAIL: Table has type tsrange, but query expects _confidentialitycode.
@@ -629,7 +662,4 @@
SET SESSION AUTHORIZATION sigmund;
SET
SELECT * FROM test;
- id | classcode | moodcode | code | confidentialitycode | effectivetime
-+---+--+--+-+---
-(0 rows)
-
+psql:/home/m/minirim2.sql:439: connection to server was lost
regards,
Yeb Havinga
--
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] Row-security on updatable s.b. views
On 02/11/2014 08:19 PM, Yeb Havinga wrote: On 2014-02-11 12:09, Craig Ringer wrote: rls-9.4-upd-sb-views-v7 Hi Craig, I compared output of psql -ef of the minirim.sql script posted earlier in http://www.postgresql.org/message-id/52f54927.1040...@gmail.com between v4 and v7. Not everything is ok. +psql:/home/m/minirim2.sql:409: ERROR: attribute 6 has wrong type +DETAIL: Table has type tsrange, but query expects _confidentialitycode. That's downright strange. I'll need to look into that one. +psql:/home/m/minirim2.sql:439: connection to server was lost I've seen a server crash for causes as yet unknown in the main regression tests too. I'd love to stick with the in-optimizer approach used in v4, which - after all - works. The trouble is that it cannot support row-security quals that incorporate views correctly. I would have to invoke the rewriter from the optimizer and deal with recursion detection to make that work, and it looks pretty ugly. -- Craig Ringer 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
From: Andres Freund and...@2ndquadrant.com which means they manipulate the lwWaitLink queue without protection. That's done intentionally. The code tries to protect against corruption of the list to do a woken up backend acquiring a lock (this or an independent one) by only continuing when the lwWaiting flag is set to false. Unfortunately there's absolutely no guarantee that a) the assignment to lwWaitLink and lwWaiting are done in that order b) that the stores are done in-order from the POV of other backends. So what we need to do is to acquire a write barrier between the assignments to lwWaitLink and lwWaiting, i.e. proc-lwWaitLink = NULL; pg_write_barrier(); proc-lwWaiting = false; the reader side already uses an implicit barrier by using spinlocks. I've got a report from one customer that they encountered a hang during performance benchmarking. They were using PostgreSQL 9.2.4. I remember that the stack trace showed many backends blocked forever at LWLockAcuuire() during btree insert operation. I'm not sure this has something to do with what you are raising, but the release notes for 9.2.5/6 doesn't suggest any fixes for this. So I felt there is something wrong with lwlocks. Do you think that your question could cause my customer's problem -- backends block at lwlock forever? 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 2014-02-11 21:46:04 +0900, MauMau wrote: From: Andres Freund and...@2ndquadrant.com which means they manipulate the lwWaitLink queue without protection. That's done intentionally. The code tries to protect against corruption of the list to do a woken up backend acquiring a lock (this or an independent one) by only continuing when the lwWaiting flag is set to false. Unfortunately there's absolutely no guarantee that a) the assignment to lwWaitLink and lwWaiting are done in that order b) that the stores are done in-order from the POV of other backends. So what we need to do is to acquire a write barrier between the assignments to lwWaitLink and lwWaiting, i.e. proc-lwWaitLink = NULL; pg_write_barrier(); proc-lwWaiting = false; the reader side already uses an implicit barrier by using spinlocks. I've got a report from one customer that they encountered a hang during performance benchmarking. They were using PostgreSQL 9.2.4. I remember that the stack trace showed many backends blocked forever at LWLockAcuuire() during btree insert operation. I'm not sure this has something to do with what you are raising, but the release notes for 9.2.5/6 doesn't suggest any fixes for this. So I felt there is something wrong with lwlocks. Do you think that your question could cause my customer's problem -- backends block at lwlock forever? It's x86, right? Then it's unlikely to be actual unordered memory accesses, but if the compiler reordered: LOG_LWDEBUG(LWLockRelease, T_NAME(l), T_ID(l), release waiter); proc = head; head = proc-lwWaitLink; proc-lwWaitLink = NULL; proc-lwWaiting = false; PGSemaphoreUnlock(proc-sem); to LOG_LWDEBUG(LWLockRelease, T_NAME(l), T_ID(l), release waiter); proc = head; proc-lwWaiting = false; head = proc-lwWaitLink; proc-lwWaitLink = NULL; PGSemaphoreUnlock(proc-sem); which it is permitted to do, yes, that could cause symptoms like you describe. Any chance you have the binaries the customer ran back then around? Disassembling that piece of code might give you a hint whether that's a possible cause. 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] Patch: show xid and xmin in pg_stat_activity and pg_stat_replication
On Fri, Feb 7, 2014 at 4:34 AM, Christian Kruse christ...@2ndquadrant.com wrote: attached you will find a new version with the following issues resolved: - use backend ID once again for getting the xid and xmin - use xref instead of link in documentation - rename fields to backend_xid and backend_xmin This looks mostly good but it doesn't address this point, from one of my earlier emails: If I understand correctly, modifying PgBackendStatus adds additional fields to the shared memory data structure that are never used and will be returned by functions like pgstat_fetch_stat_beentry() unitialized. That seems both inefficient and a pitfall for the unwary. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] narwhal and PGDLLIMPORT
On Tue, Feb 11, 2014 at 11:01 AM, Craig Ringer cr...@2ndquadrant.com wrote: On 02/11/2014 01:28 PM, Tom Lane wrote: If there are no objections, I'll push this patch into HEAD tomorrow, along with the upthread patches from Craig Ringer and Marco Atzeri. We might as well see if this stuff is going to work ... I'd love to test my patch properly before pushing it, but my dev machine is going to need a total teardown and rebuild, I can do the test of your patch/idea, please confirm if below steps are sufficient: a. Change manually postgres.def file and add DATA for MainLWLockArray. (Will it be sufficient to change manually or should I apply your patch) b. Rebuild pg_buffercache module c. Test pg_buffercache if it can access the variable. d. If above works, then run 'check'. If I understand correctly your patch is intended to resolve PGDLLIMPORT problem, right? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] encouraging index-only scans
On Mon, Feb 3, 2014 at 11:55:34AM -0500, Robert Haas wrote: Robert, where are we on this? Should I post a patch? I started working on this at one point but didn't finish the implementation, let alone the no-doubt-onerous performance testing that will be needed to validate whatever we come up with. It would be really easy to cause serious regressions with ill-considered changes in this area, and I don't think many people here have the bandwidth for a detailed study of all the different workloads that might be affected here right this very minute. More generally, you're sending all these pings three weeks after the deadline for CF4. I don't think that's a good time to encourage people to *start* revising old patches, or writing new ones. I've also had some further thoughts about the right way to drive vacuum scheduling. I think what we need to do is tightly couple the I understand the problems with vacuum scheduling, but I was trying to address _just_ the insert-only workload problem for index-only scans. Right now, as I remember, only vacuum sets the visibility bits. If we don't want to make vacuum trigger for insert-only workloads, can we set pages all-visible more often? Is there a reason that a sequential scan, which does do page pruning, doesn't set the visibility bits too? Or does it? Can an non-index-only index scan that finds the heap tuple all-visible and the page not all-visible check the other items on the page to see if the page can be marked all-visible? Does analyze set pages all-visible? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] Changeset Extraction v7.5
On Fri, Feb 7, 2014 at 2:35 PM, Andres Freund and...@2ndquadrant.com wrote: attached you can find the next version of the patchset. As usual, I'm going to be reviewing patch 1. The definition of patch 1 has changed quite a few times over the past year, but that's usually the one I'm reviewing. + * contents of records in here xexcept turning them into a more usable Typo. + /* +* XXX: There doesn't seem to be a usecase for decoding +* HEAP_NEWPAGE's. Its only used in various indexam's and CLUSTER, +* neither of which should be relevant for the logical +* changestream. +*/ There's a level of uncertainty here that doesn't seem consistent with calling this a finished patch. It's also not a complete list of places where log_newpage() is called, but frankly I don't think that should be the aim of this comment. The only relevant question is whether we ever use XLOG_HEAP_NEWPAGE to log heap changes that are relevant to logical replication. I think we don't. + /* FIXME: skip if wrong db? */ It's time to fish or cut bait. + /* +* XXX: As a future feature, we could replay the transaction and +* prepare it as well, allowing for 2PC via logical decoding. +*/ Let's try to avoid using XXX (or FIXME) for things that really mean TODO. I think this comment deserves to be expanded a bit, too. Maybe something like: Right now, logical decoding ignores PREPARE TRANSACTION and simply decodes the subsequent COMMIT TRANSACTION or ROLLBACK TRANSACTION just as it would a regular COMMIT or ROLLBACK. In the future, we might want to change this. Decoding PREPARE might enable future code to prepare each locally prepared transaction on the remote side before doing a COMMIT TRANSACTION locally, allowing for logical synchronous replication. + /* +* If the record wasn't part of a transaction, it will not have +* caused invalidations and thus isn't important when building +* snapshots. If it was part of a transaction, that transaction +* just performed DDL because those are the only codepaths using +* inplace updates. +*/ Under what circumstances do we issue in-place updates not associated with a transaction? And under what circumstances do we issue in-place updates that ARE associated with a transaction? +* XXX: At some point we might want to execute the transaction's The XXX again seems needless; the comment is fine as it stands. + /* +* Abort all transactions that we keep track of that are older +* than -oldestRunningXid. This is the most convenient spot I think writing -membername is poor commenting style. Just leave out the arrow, or write the WAL record's oldestRunningXid. +/* + * Get the data from the various forms of commit records and pass it + * on to snapbuild.c and reorderbuffer.c + */ This is a lousy comment. I suggest something like: Currently, each transaction is decoded only once it commit, so the arrival of a commit record means that we can now decode the changes made by this toplevel transaction and all of its committed subtransactions, unless we have to skip it because the replication system isn't fully initialized yet. Whether decoding the transaction or not, we must take note of any invalidations it issues, as those will affect the snapshot used for decoding of *other* transactions. +/* + * Get the data from the various forms of abort records and pass it on to + * snapbuild.c and reorderbuffer.c + */ Suggest: When a transaction abort is detected, we throw away any data we've stashed away for possible future decoding of that transaction. Knowledge of the abort may also help us establish our initial snapshot when logical decoding is first initiated. +/* + * Set the xmin required for decoding snapshots for the specific decoding + * slot. + */ +void +IncreaseLogicalXminForSlot(XLogRecPtr lsn, TransactionId xmin) I'm thinking this and everything that follows, up through LogicalDecodingCountDBSlots, probably should be moved to slot.c. + /* XXX: Add the current LSN? */ +1. + /* shorter lines... */ + slot = MyReplicationSlot; If you're going to do this, which seems like it's probably a good idea, do it at the top of the function and use it all the way through instead of doing it in the middle. + if (MyReplicationSlot == NULL) + elog(ERROR, need a current slot); + + if (is_init start_lsn != InvalidXLogRecPtr) + elog(ERROR, Cannot INIT_LOGICAL_REPLICATION at a specified LSN); + + if (is_init
Re: [HACKERS] Recovery inconsistencies, standby much larger than primary
On Sun, Feb 9, 2014 at 2:54 PM, Greg Stark st...@mit.edu wrote: Bad block's page header -- this is in the 56'th relation segment: =# select (page_header(E'\\x2005583b05aa050028001805002004201098e00f2090e00f088d24061885e00f')).*; lsn | tli | flags | lower | upper | special | pagesize | version | prune_xid --+-+---+---+---+-+--+-+--- 520/AA053B58 | 5 | 0 |40 | 1304 |8192 | 8192 | 4 | 0 Looking at the block at offset 4773121 (which is in the 36th segment): ... d9de7pcqls4ib6=# select (page_header(E'\\x2005a00a0bad05002c00a002002004201098e00f2090e00f088d24061885e00fa082ec04')).*; lsn | tli | flags | lower | upper | special | pagesize | version | prune_xid --+-+---+---+---+-+--+-+--- 520/AD0B0AA0 | 5 | 0 |44 | 672 |8192 | 8192 | 4 | 0 (1 row) And I finally tracked down the xlog record for this stray write: [cur:520/AA051FE0, xid:7635428, rmid:10(Heap), len/tot_len:21/7005, info:8, prev:520/AA051F20] insert: s/d/r:1663/16385/16650 blk/off:4773121/4 header: none [cur:520/AA051FE0, xid:7635428, rmid:10(Heap), len/tot_len:21/7005, info:8, prev:520/AA051F20] bkpblock[1]: s/d/r:1663/16385/16650 blk:4773121 hole_off/len:40/1264 -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] encouraging index-only scans
On Tue, Feb 11, 2014 at 10:56 AM, Bruce Momjian br...@momjian.us wrote: Right now, as I remember, only vacuum sets the visibility bits. If we don't want to make vacuum trigger for insert-only workloads, can we set pages all-visible more often? Is there a reason that a sequential scan, which does do page pruning, doesn't set the visibility bits too? Or does it? Can an non-index-only index scan that finds the heap tuple all-visible and the page not all-visible check the other items on the page to see if the page can be marked all-visible? Does analyze set pages all-visible? A sequential scan will set hint bits and will prune the page, but pruning the page doesn't ever mark it all-visible; that logic is entirely in vacuum. If that could be made cheap enough to be negligible, it might well be worth doing in heap_page_prune(). I think there might be a way to do that, but it's a bit tricky because the pruning logic iterates over the page in a somewhat complex way, not just a straightforward scan of all the item pointers the way the existing logic doesn't. It would be pretty cool if we could just use a bit out of the heap-prune xlog record to indicate whether the all-visible bit should be set; then we'd gain the benefit of marking things all-visible much more often without needing vacuum. That doesn't help insert-only tables much, though, because those won't require pruning. We set hint bits (which dirties the page) but currently don't write WAL. We'd have to change that to set the all-visible bit when scanning such a table, and that would be expensive. :-( -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: tests for client programs
On Sat, Feb 8, 2014 at 10:16 PM, Peter Eisentraut pete...@gmx.net wrote: Clearly, we will need to figure out something about how to require this module, and possibly others in the future, as we expand the tests. Having configure check for it is not necessarily the best solution -- What is configure supposed to do if it can't find it? We could perhaps use Test::More skip_all to just skip these tests depending on what modules are available. And add appropriate documentation. I would think we would want to keep the number of dependencies relatively small. If it gets large, that just means that nobody will be able to run the tests. And -1 for the idea of running only the tests that we can given what's installed; that'll make it very easy to not run all the tests, which kind of defeats the purpose of having them IMHO. We should just require whatever we need and let the test run abort if it's not available. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Performance Improvement by reducing WAL for Update Operation
On Wed, Feb 5, 2014 at 10:57:57AM -0800, Peter Geoghegan wrote: On Wed, Feb 5, 2014 at 12:50 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: I think there's zero overlap. They're completely complimentary features. It's not like normal WAL records have an irrelevant volume. Correct. Compressing a full-page image happens on the first update after a checkpoint, and the diff between old and new tuple is not used in that case. Uh, I really just meant that one thing that might overlap is considerations around the choice of compression algorithm. I think that there was some useful discussion of that on the other thread as well. Yes, that was my point. I though the compression of full-page images was a huge win and that compression was pretty straight-forward, except for the compression algorithm. If the compression algorithm issue is resolved, can we move move forward with the full-page compression patch? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] [PERFORM] encouraging index-only scans
On Tue, Feb 11, 2014 at 11:28:36AM -0500, Robert Haas wrote: A sequential scan will set hint bits and will prune the page, but pruning the page doesn't ever mark it all-visible; that logic is entirely in vacuum. If that could be made cheap enough to be negligible, it might well be worth doing in heap_page_prune(). I think there might be a way to do that, but it's a bit tricky because the pruning logic iterates over the page in a somewhat complex way, not just a straightforward scan of all the item pointers the way the existing logic doesn't. It would be pretty cool if we could just use a bit out of the heap-prune xlog record to indicate whether the all-visible bit should be set; then we'd gain the benefit of marking things all-visible much more often without needing vacuum. That doesn't help insert-only tables much, though, because those won't require pruning. We set hint bits (which dirties the page) but currently don't write WAL. We'd have to change that to set the all-visible bit when scanning such a table, and that would be expensive. :-( Yes, that pretty much sums it up. We introduced index-only scans in 9.2 (2012) but they still seem to be not usable for insert-only workloads two years later. Based on current progress, it doesn't look like this will be corrected until 9.5 (2015). I am kind of confused why this has not generated more urgency. I guess my question is what approach do we want to take to fixing this? If we are doing pruning, aren't we emitting WAL? You are right that for an insert-only workload, we aren't going to prune, but if pruning WAL overhead is acceptable for a sequential scan, isn't index-only page-all-visible WAL overhead acceptable? Do we want to track the number of inserts in statistics and trigger an auto-vacuum after a specified number of inserts? The problem there is that we really don't need to do any index cleanup, which is what vacuum typically does --- we just want to scan the table and set the all-visible bits, so that approach seems non-optimal. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] narwhal and PGDLLIMPORT
Marco Atzeri marco.atz...@gmail.com writes: On 09/02/2014 14:10, Andrew Dunstan wrote: On 02/09/2014 01:12 AM, Marco Atzeri wrote: we should have get rid of dlltool on cygwin. At least it is not used on my build The send in a patch. The patch you sent in previously did not totally remove it IIRC. attached patch versus latest git. I've committed this with some fixes. However, I omitted the hunks that change the names of generated shared libraries (to add SO_MAJOR_VERSION). I think that requires some policy discussion around whether we want to do it or not, and in any case it's unrelated to the issues being discussed in this thread. If you still want that, please submit it as a separate patch in a new thread, with some discussion as to why it's a good idea. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] encouraging index-only scans
On 2014-02-11 12:12:13 -0500, Bruce Momjian wrote: Yes, that pretty much sums it up. We introduced index-only scans in 9.2 (2012) but they still seem to be not usable for insert-only workloads two years later. Based on current progress, it doesn't look like this will be corrected until 9.5 (2015). I am kind of confused why this has not generated more urgency. I think this largely FUD. They are hugely beneficial in some scenarios and less so in others. Just like lots of other features we have. 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] Changeset Extraction v7.5
Hi! On 2014-02-11 11:22:24 -0500, Robert Haas wrote: + * contents of records in here xexcept turning them into a more usable Typo. + /* +* XXX: There doesn't seem to be a usecase for decoding +* HEAP_NEWPAGE's. Its only used in various indexam's and CLUSTER, +* neither of which should be relevant for the logical +* changestream. +*/ There's a level of uncertainty here that doesn't seem consistent with calling this a finished patch. It's also not a complete list of places where log_newpage() is called, but frankly I don't think that should be the aim of this comment. The only relevant question is whether we ever use XLOG_HEAP_NEWPAGE to log heap changes that are relevant to logical replication. I think we don't. You're right, we currently don't. I guess we should add a comment to log_newpage()/buffer to make sure that's not violated in future code, that seems like the only place that's somewhat likely to be read? Decoding PREPARE might enable future code to prepare each locally prepared transaction on the remote side before doing a COMMIT TRANSACTION locally, allowing for logical synchronous replication. We do support logical synchronous replication over the walsender interface, what it would allow us to do would be some form of 2pc... I'll adapt your version. + /* +* If the record wasn't part of a transaction, it will not have +* caused invalidations and thus isn't important when building +* snapshots. If it was part of a transaction, that transaction +* just performed DDL because those are the only codepaths using +* inplace updates. +*/ Under what circumstances do we issue in-place updates not associated with a transaction? And under what circumstances do we issue in-place updates that ARE associated with a transaction? vacuum updates relfrozenxid outside a transaction, e.g. create/drop index, analyze inside one. +/* + * Get the data from the various forms of commit records and pass it + * on to snapbuild.c and reorderbuffer.c + */ This is a lousy comment. I suggest something like: Currently, each transaction is decoded only once it commit, so the arrival of a commit record means that we can now decode the changes made by this toplevel transaction and all of its committed subtransactions, unless we have to skip it because the replication system isn't fully initialized yet. Whether decoding the transaction or not, we must take note of any invalidations it issues, as those will affect the snapshot used for decoding of *other* transactions. +/* + * Get the data from the various forms of abort records and pass it on to + * snapbuild.c and reorderbuffer.c + */ Suggest: When a transaction abort is detected, we throw away any data we've stashed away for possible future decoding of that transaction. Knowledge of the abort may also help us establish our initial snapshot when logical decoding is first initiated. Hm, those should go into reorderbuffer.c instead, the interesting part of DecodeCommit/DecodeAbort is just to centralize the handling of the various forms of commit/abort records. decode.c really shouldn't need to be changed much when we start to optionally support streaming out changes immediately. + /* register output plugin name with slot */ + strncpy(NameStr(MyReplicationSlot-data.plugin), plugin, + NAMEDATALEN); + NameStr(MyReplicationSlot-data.plugin)[NAMEDATALEN - 1] = '\0'; Hmm. Shouldn't this be delegated to something in slot.c? Why don't we need the lock? I wasn't sure, we can place it there, but it doesn't really need to know about these details either. Lockingwise I don't see it needing more, nobody but the slot that has it acquired is interested in it. Why don't we need to fsync() the change? I've since pushed a patch that does the fsyncing. Not doing so was part of a rebase screwup. + /* +* FIXME: we're going to have to do something more intelligent about +* timelines on standby's. Use readTimeLineHistory() and +* tliOfPointInHistory() to get the proper LSN? +*/ So what's the plan for that? Prohibit decoding on the standby for now. Not sure how to deal with the relevant code, leave it there, #ifdef it out, remove it? + /* +* XXX: It'd be way nicer to be able to use the walsender waiting logic +* here, but that's not available in all environments. +*/ I don't understand this. The walsender get's notified when flushing WAL, which allows the walsender specific
Re: [HACKERS] jsonb and nested hstore
On Tue, Feb 11, 2014 at 3:35 AM, Hannu Krosing ha...@2ndquadrant.com wrote: On 02/11/2014 01:16 AM, Merlin Moncure wrote: On Mon, Feb 10, 2014 at 5:52 PM, Andres Freund and...@2ndquadrant.com wrote: It works in enough cases atm that it's worthwile trying to keep it working. Sure, it could be better, but it's what we have right now. Atm it's e.g. the only realistic way to copy larger amounts of bytea between servers without copying the entire cluster. That's the thing -- it might work today, but what about tomorrow? We'd be sending the wrong signals. People start building processes around all of this and now we've painted ourselves into a box. Better in my mind to simply educate users that this practice is dangerous and unsupported, as we used to do. I guess until now. It seems completely odd to me that we're attaching a case to the jsonb type, in the wrong way -- something that we've never attached to any other type before. For example, why didn't we attach a version code to the json type send function? JSON is supposed to be a *standard* way of encoding data in strings. If the ever changes, it will not be JSON type anymore. My point was that as we reserved the right to change jsonb binary format we'd probably want to reserve the right to change json's as well. This was in support of the theme of 'why is jsonb a special case?'. However, I think it's pretty much settled that the any potential concerns I raised in terms of providing a version flag are outweighed by it's potential usefulness. merlin -- 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] narwhal and PGDLLIMPORT
Hiroshi Inoue in...@tpf.co.jp writes: (2014/02/09 8:06), Andrew Dunstan wrote: Yeah. Incidentally, we didn't quite get rid of dlltool for Cygwin. We did get rid of dllwrap. But I agree this is worth trying for Mingw. I tried MINGW port with the attached change and successfully built src and contrib and all pararell regression tests were OK. I cleaned this up a bit (the if-nesting in Makefile.shlib was making my head hurt, not to mention that it left a bunch of dead code) and committed it. By my count, the only remaining usage of dlltool is in plpython's Makefile. Can we get rid of that? Also, the only remaining usage of dllwrap is in src/bin/pgevent/Makefile. Do we need that either? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] encouraging index-only scans
On Tue, Feb 11, 2014 at 06:54:10PM +0100, Andres Freund wrote: On 2014-02-11 12:12:13 -0500, Bruce Momjian wrote: Yes, that pretty much sums it up. We introduced index-only scans in 9.2 (2012) but they still seem to be not usable for insert-only workloads two years later. Based on current progress, it doesn't look like this will be corrected until 9.5 (2015). I am kind of confused why this has not generated more urgency. I think this largely FUD. They are hugely beneficial in some scenarios and less so in others. Just like lots of other features we have. I don't understand. Index-only scans are known to have benefits --- if an insert-only workload can't use that, why is that acceptable? What is fear-uncertainty-and-doubt about that? Please explain. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] [PERFORM] encouraging index-only scans
On 2014-02-11 13:23:19 -0500, Bruce Momjian wrote: On Tue, Feb 11, 2014 at 06:54:10PM +0100, Andres Freund wrote: On 2014-02-11 12:12:13 -0500, Bruce Momjian wrote: Yes, that pretty much sums it up. We introduced index-only scans in 9.2 (2012) but they still seem to be not usable for insert-only workloads two years later. Based on current progress, it doesn't look like this will be corrected until 9.5 (2015). I am kind of confused why this has not generated more urgency. I think this largely FUD. They are hugely beneficial in some scenarios and less so in others. Just like lots of other features we have. I don't understand. Index-only scans are known to have benefits --- if an insert-only workload can't use that, why is that acceptable? What is fear-uncertainty-and-doubt about that? Please explain. Uh, for one, insert only workloads certainly aren't the majority of usecases. Ergo there are plenty of cases where index only scans work out of the box. Also, they *do* work for insert only workloads, you just either have to wait longer, or manually trigger VACUUMs. That's a far cry from not being usable. I am not saying it shouldn't be improved, I just don't see the point of bringing it up while everyone is busy with the last CF and claiming it is unusable and that stating that it is surprisising that nobody really cares. 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] unitialized data in populate_record_worker
Hi, while testing a patch I ran valgrind over a recent checkout, and it spit out the following: ==14792== Conditional jump or move depends on uninitialised value(s) ==14792==at 0x7F8A30: populate_record_worker (jsonfuncs.c:1459) ==14792==by 0x7F8451: json_to_record (jsonfuncs.c:1280) ==14792==by 0x632EF2: ExecMakeTableFunctionResult (execQual.c:2164) ==14792==by 0x65387B: FunctionNext (nodeFunctionscan.c:94) ==14792==by 0x63A209: ExecScanFetch (execScan.c:82) ==14792==by 0x63A278: ExecScan (execScan.c:132) ==14792==by 0x653BD0: ExecFunctionScan (nodeFunctionscan.c:266) ==14792==by 0x62F2B6: ExecProcNode (execProcnode.c:426) ==14792==by 0x62D070: ExecutePlan (execMain.c:1474) ==14792==by 0x62B1CE: standard_ExecutorRun (execMain.c:308) ==14792==by 0x62B045: ExecutorRun (execMain.c:256) ==14792==by 0x7850B6: PortalRunSelect (pquery.c:946) which looks accurate, my_extra-columns doesn't look initialized. 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] [PERFORM] encouraging index-only scans
On Tue, Feb 11, 2014 at 07:31:03PM +0100, Andres Freund wrote: On 2014-02-11 13:23:19 -0500, Bruce Momjian wrote: On Tue, Feb 11, 2014 at 06:54:10PM +0100, Andres Freund wrote: On 2014-02-11 12:12:13 -0500, Bruce Momjian wrote: Yes, that pretty much sums it up. We introduced index-only scans in 9.2 (2012) but they still seem to be not usable for insert-only workloads two years later. Based on current progress, it doesn't look like this will be corrected until 9.5 (2015). I am kind of confused why this has not generated more urgency. I think this largely FUD. They are hugely beneficial in some scenarios and less so in others. Just like lots of other features we have. I don't understand. Index-only scans are known to have benefits --- if an insert-only workload can't use that, why is that acceptable? What is fear-uncertainty-and-doubt about that? Please explain. Uh, for one, insert only workloads certainly aren't the majority of usecases. Ergo there are plenty of cases where index only scans work out of the box. True. Also, they *do* work for insert only workloads, you just either have to wait longer, or manually trigger VACUUMs. That's a far cry from not Wait longer for what? Anti-xid-wraparound vacuum? being usable. Is using VACUUM for these cases documented? Should it be? I am not saying it shouldn't be improved, I just don't see the point of bringing it up while everyone is busy with the last CF and claiming it is unusable and that stating that it is surprisising that nobody really cares. Well, I brought it up in September too. My point was not that it is a new issue but that it has been such an ignored issue for two years. I am not asking for a fix, but right now we don't even have a plan on how to improve this. I still don't see how this is FUD, and you have not explained it to me. This is a known limitation for two years, not documented (?), and with no TODO item and no plan on how to improve it. Do you want to declare such cases FUD and just ignore them? I don't see how that moves us forward. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] narwhal and PGDLLIMPORT
Craig Ringer cr...@2ndquadrant.com writes: For MSVC, here's a patch that makes gendef.pl emit DATA annotations for global var exports. Committed. Also attached is a patch to make vcregress.pl produce a better error message when there's no build output, instead of just reporting that .. is not a recognized internal or external command, operable program, or batch file I would've committed this, but I'm a bit hesitant about this part: + use 5.10.1; Are we moving the goalposts on the Perl version needed, and if so how far? I didn't question the use 5.8.0 you added to gendef.pl, but it disturbs me that this isn't the same. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] encouraging index-only scans
Bruce Momjian br...@momjian.us writes: On Tue, Feb 11, 2014 at 07:31:03PM +0100, Andres Freund wrote: I am not saying it shouldn't be improved, I just don't see the point of bringing it up while everyone is busy with the last CF and claiming it is unusable and that stating that it is surprisising that nobody really cares. Well, I brought it up in September too. My point was not that it is a new issue but that it has been such an ignored issue for two years. I am not asking for a fix, but right now we don't even have a plan on how to improve this. Indeed, and considering that we're all busy with the CF, I think it's quite unreasonable of you to expect that we'll drop everything else to think about this problem right now. The reason it's like it is is that it's not easy to see how to make it better; so even if we did drop everything else, it's not clear to me that any plan would emerge anytime soon. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] encouraging index-only scans
On 2014-02-11 13:41:46 -0500, Bruce Momjian wrote: Wait longer for what? Anti-xid-wraparound vacuum? Yes. Is using VACUUM for these cases documented? Should it be? No idea, it seems to be part of at least part of the folkloric knowledge, from what I see at clients. I am not saying it shouldn't be improved, I just don't see the point of bringing it up while everyone is busy with the last CF and claiming it is unusable and that stating that it is surprisising that nobody really cares. Well, I brought it up in September too. My point was not that it is a new issue but that it has been such an ignored issue for two years. I am not asking for a fix, but right now we don't even have a plan on how to improve this. Coming up with a plan for this takes time and discussion, not something we seem to have aplenty of atm. And even if were to agree on a plan right now, we wouldn't incorporate it into 9.4, so what's the point of bringing it up now? I still don't see how this is FUD, and you have not explained it to me. This is a known limitation for two years, not documented (?), and with no TODO item and no plan on how to improve it. Do you want to declare such cases FUD and just ignore them? I don't see how that moves us forward. Claiming something doesn't work while it just has manageable usability issues doesn't strike me as a reasonable starting point. If it bugs somebody enough to come up with a rough proposal it will get fixed... 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] [PERFORM] encouraging index-only scans
On Tue, Feb 11, 2014 at 01:54:48PM -0500, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: On Tue, Feb 11, 2014 at 07:31:03PM +0100, Andres Freund wrote: I am not saying it shouldn't be improved, I just don't see the point of bringing it up while everyone is busy with the last CF and claiming it is unusable and that stating that it is surprisising that nobody really cares. Well, I brought it up in September too. My point was not that it is a new issue but that it has been such an ignored issue for two years. I am not asking for a fix, but right now we don't even have a plan on how to improve this. Indeed, and considering that we're all busy with the CF, I think it's quite unreasonable of you to expect that we'll drop everything else to think about this problem right now. The reason it's like it is is that it's not easy to see how to make it better; so even if we did drop everything else, it's not clear to me that any plan would emerge anytime soon. Well, documenting the VACUUM requirement and adding it to the TODO list are things we should consider for 9.4. If you think doing that after the commit-fest is best, I can do that. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] [PERFORM] encouraging index-only scans
On Tue, Feb 11, 2014 at 9:12 AM, Bruce Momjian br...@momjian.us wrote: On Tue, Feb 11, 2014 at 11:28:36AM -0500, Robert Haas wrote: A sequential scan will set hint bits and will prune the page, but pruning the page doesn't ever mark it all-visible; that logic is entirely in vacuum. If that could be made cheap enough to be negligible, it might well be worth doing in heap_page_prune(). I think there might be a way to do that, but it's a bit tricky because the pruning logic iterates over the page in a somewhat complex way, not just a straightforward scan of all the item pointers the way the existing logic doesn't. It would be pretty cool if we could just use a bit out of the heap-prune xlog record to indicate whether the all-visible bit should be set; then we'd gain the benefit of marking things all-visible much more often without needing vacuum. That doesn't help insert-only tables much, though, because those won't require pruning. We set hint bits (which dirties the page) but currently don't write WAL. We'd have to change that to set the all-visible bit when scanning such a table, and that would be expensive. :-( Yes, that pretty much sums it up. We introduced index-only scans in 9.2 (2012) but they still seem to be not usable for insert-only workloads two years later. Based on current progress, it doesn't look like this will be corrected until 9.5 (2015). I am kind of confused why this has not generated more urgency. For insert and select only, they are usable (if your queries are of the type that could benefit from them), you just have to do some manual intervention. The list of features that sometimes require a DBA to do something to make maximum use of them under some circumstance would be a long one. It would be nice if it were better, but I don't see why this feature is particularly urgent compared to all the other things that could be improved. In particular I think the Freezing without IO is much more important. Freezing is rather unimportant until suddenly it is is the most important thing in the universe. If we could stop worrying about that, I think it would free up other aspects of vacuum scheduling to have more meddling/optimization done to it. I guess my question is what approach do we want to take to fixing this? If we are doing pruning, aren't we emitting WAL? You are right that for an insert-only workload, we aren't going to prune, but if pruning WAL overhead is acceptable for a sequential scan, isn't index-only page-all-visible WAL overhead acceptable? We often don't find that pruning particularly acceptable in seq scans, and there is a patch pending to conditionally turn it off for them. Do we want to track the number of inserts in statistics and trigger an auto-vacuum after a specified number of inserts? We track relpages and relallvisible, which seems like a more direct measure. Once analyze is done (which is already triggered by inserts) and sets those, it could fire a vacuum based on the ratio of those values, or the autovac process could just look at the ratio after naptime. So just introduce autovacuum_vacuum_visible_factor. A problem there is that it would be a lot of work to aggressively keep the ratio high, and pointless if the types of queries done on that table don't benefit from IOS anyway, or if pages are dirtied so rapidly that no amount of vacuuming will keep the ratio high. Would we try to automatically tell which tables were which, or rely on the DBA setting per-table autovacuum_vacuum_visible_factor for tables that differ from the database norm? The problem there is that we really don't need to do any index cleanup, which is what vacuum typically does --- we just want to scan the table and set the all-visible bits, so that approach seems non-optimal. In the case of no updates or deletes (or aborted inserts?), there would be nothing to clean up in the indexes and that step would be skipped (already in the current code). And if the indexes do need cleaning up, we certainly can't set the page all visible without doing that clean up. Cheers, Jeff
Re: [HACKERS] PostgreSQL Failback without rebuild
On Fri, Feb 7, 2014 at 03:57:31PM +1100, James Sewell wrote: I've just noticed that on PostgreSQL 9.3 I can do the following with a master node A and a slave node B (as long as I have set recovery_target_timeline = 'latest'): 1. Stop Node A 2. Promote Node B 3. Attach Node A as slave This is sufficient for my needs (I know it doesn't cover a crash), can anyone see any potential problems with this approach? I know some people have used rsync to get A to match B after a crash of A and promotion of B. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] [PERFORM] encouraging index-only scans
On Tue, Feb 11, 2014 at 4:13 PM, Jeff Janes jeff.ja...@gmail.com wrote: Do we want to track the number of inserts in statistics and trigger an auto-vacuum after a specified number of inserts? We track relpages and relallvisible, which seems like a more direct measure. Once analyze is done (which is already triggered by inserts) and sets those, it could fire a vacuum based on the ratio of those values, or the autovac process could just look at the ratio after naptime. So just introduce autovacuum_vacuum_visible_factor. A problem there is that it would be a lot of work to aggressively keep the ratio high, and pointless if the types of queries done on that table don't benefit from IOS anyway, or if pages are dirtied so rapidly that no amount of vacuuming will keep the ratio high. Would we try to automatically tell which tables were which, or rely on the DBA setting per-table autovacuum_vacuum_visible_factor for tables that differ from the database norm? Why not track how many times an IOS would be used but wasn't, or how many heap fetches in IOS have to be performed? Seems like a more direct measure of whether allvisible needs an update. -- 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] narwhal and PGDLLIMPORT
On 11/02/2014 18:15, Tom Lane wrote: Marco Atzeri marco.atz...@gmail.com writes: On 09/02/2014 14:10, Andrew Dunstan wrote: On 02/09/2014 01:12 AM, Marco Atzeri wrote: we should have get rid of dlltool on cygwin. At least it is not used on my build The send in a patch. The patch you sent in previously did not totally remove it IIRC. attached patch versus latest git. I've committed this with some fixes. However, I omitted the hunks that change the names of generated shared libraries (to add SO_MAJOR_VERSION). I think that requires some policy discussion around whether we want to do it or not, and in any case it's unrelated to the issues being discussed in this thread. If you still want that, please submit it as a separate patch in a new thread, with some discussion as to why it's a good idea. regards, tom lane Noted. On cygwin the shared libraries are using the SO_MAJOR_VERSION by long time. cd /usr/bin $ ls cyggcc*dll cyggcc_s-1.dll cyggccpp-1.dll $ ls cygfo*dll cygfontconfig-1.dll cygform-10.dll cygform-8.dll cygformw-10.dll cygfontenc-1.dll cygform7.dllcygform-9.dll In this way we allow coexistence of several release, similar to /usr/lib/libpq.so.5 on unix. Regards Marco -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Unhappy with error handling in psql's handleCopyOut()
While looking at the pending patch to make psql report a line count after COPY, I came across this business in handleCopyOut(): * Check command status and return to normal libpq state. After a * client-side error, the server will remain ready to deliver data. The * cleanest thing is to fully drain and discard that data. If the * client-side error happened early in a large file, this takes a long * time. Instead, take advantage of the fact that PQexec() will silently * end any ongoing PGRES_COPY_OUT state. This does cause us to lose the * results of any commands following the COPY in a single command string. * It also only works for protocol version 3. XXX should we clean up * using the slow way when the connection is using protocol version 2? which git blames on commit 08146775 (committed by Alvaro on behalf of Noah). This does not make me happy. In the first place, we have not dropped support for protocol version 2. In the second place, I fail to see what the advantage is of kluging things like this. The main costs of draining the remaining COPY data are the server-side work of generating the data and the network transmission costs, neither of which will go away with this technique. So I'm thinking we should revert this kluge and just drain the data straightforwardly, which would also eliminate the mentioned edge-case misbehavior when there were more commands in the query string. Is there a reason I'm overlooking not to do this? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unhappy with error handling in psql's handleCopyOut()
* Tom Lane (t...@sss.pgh.pa.us) wrote:
While looking at the pending patch to make psql report a line count
after COPY, I came across this business in handleCopyOut():
* Check command status and return to normal libpq state. After a
* client-side error, the server will remain ready to deliver data. The
* cleanest thing is to fully drain and discard that data. If the
* client-side error happened early in a large file, this takes a long
* time. Instead, take advantage of the fact that PQexec() will silently
* end any ongoing PGRES_COPY_OUT state. This does cause us to lose the
* results of any commands following the COPY in a single command string.
* It also only works for protocol version 3. XXX should we clean up
* using the slow way when the connection is using protocol version 2?
which git blames on commit 08146775 (committed by Alvaro on behalf of
Noah).
This does not make me happy. In the first place, we have not dropped
support for protocol version 2. In the second place, I fail to see
what the advantage is of kluging things like this. The main costs of
draining the remaining COPY data are the server-side work of generating
the data and the network transmission costs, neither of which will go
away with this technique. So I'm thinking we should revert this kluge
and just drain the data straightforwardly, which would also eliminate
the mentioned edge-case misbehavior when there were more commands in
the query string.
Is there a reason I'm overlooking not to do this?
I've not gotten back to it yet, but I ran into a related-seeming issue
where psql would happily chew up 2G of memory trying to send COPY
failed notices when it gets disconnected from a server that it's trying
to send data to mid-COPY. conn-sock was -1, connection was
'CONNECTION_BAD', but the loop towards the end of handleCopyIn doesn't
care and nothing in libpq is changing PQresultStatus():
/*
* Check command status and return to normal libpq state
*
* We must not ever return with the status still PGRES_COPY_IN. Our
* caller is unable to distinguish that situation from reaching the next
* COPY in a command string that happened to contain two consecutive COPY
* FROM STDIN commands. XXX if something makes PQputCopyEnd() fail
* indefinitely while retaining status PGRES_COPY_IN, we get an infinite
* loop. This is more realistic than handleCopyOut()'s counterpart risk.
*/
while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN)
{
OK = false;
PQclear(res);
PQputCopyEnd(pset.db, _(trying to exit copy mode));
}
And so it would just keep looping, first building up the 2G of 'copy
failed' notices from the PQputCopyEnd, and then just spinning forever
because it couldn't drain the queue.
Thanks,
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] Patch: compiling the docs under Gentoo
On 1/30/14, 2:42 AM, Christian Kruse wrote: +Since Gentoo often supports different versions of a package to be +installed you have to tell the PostgreSQL build environment where the +Docbook DTD is located: +programlisting +cd /path/to/postgresql/sources/doc +make DOCBOOKSTYLE=/usr/share/sgml/docbook/sgml-dtd-4.2 +/programlisting This is wrong. DOCBOOKSTYLE points to the DSSSL style sheets, not the DTD. The DTD should be found automatically using the SGML catalog mechanism. (That's something that the package system should manage.) -- 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] Unhappy with error handling in psql's handleCopyOut()
Stephen Frost sfr...@snowman.net writes: I've not gotten back to it yet, but I ran into a related-seeming issue where psql would happily chew up 2G of memory trying to send COPY failed notices when it gets disconnected from a server that it's trying to send data to mid-COPY. conn-sock was -1, connection was 'CONNECTION_BAD', but the loop towards the end of handleCopyIn doesn't care and nothing in libpq is changing PQresultStatus(): [ scratches head... ] Offhand I'd have expected PQgetResult to start returning error indications. It definitely will do that if it gets error indications from the I/O layer. Perhaps it didn't see any because asyncStatus had already been reset from PGASYNC_BUSY? If so, maybe we need an explicit test on the connection being good before we return valid PGRES_COPY_IN etc results. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unhappy with error handling in psql's handleCopyOut()
* Tom Lane (t...@sss.pgh.pa.us) wrote:
Stephen Frost sfr...@snowman.net writes:
I've not gotten back to it yet, but I ran into a related-seeming issue
where psql would happily chew up 2G of memory trying to send COPY
failed notices when it gets disconnected from a server that it's trying
to send data to mid-COPY. conn-sock was -1, connection was
'CONNECTION_BAD', but the loop towards the end of handleCopyIn doesn't
care and nothing in libpq is changing PQresultStatus():
[ scratches head... ] Offhand I'd have expected PQgetResult to start
returning error indications. It definitely will do that if it gets
error indications from the I/O layer. Perhaps it didn't see any
because asyncStatus had already been reset from PGASYNC_BUSY?
The I/O layer was definitely returning errors, I traced through that
using gdb (tho I don't have it open any more, had to move on to other
things; been planning to reproduce it). Here's the full p *conn:
$9 = {pghost = 0x7fa742753300 XX, pghostaddr =
0x0, pgport = 0x7fa742753340 X, pgunixsocket = 0x0, pgtty =
0x7fa742753360 , connect_timeout = 0x0, client_encoding_initial = 0x0,
pgoptions = 0x7fa742753380 , appname = 0x0, fbappname = 0x7fa7427533a0
psql, dbName = 0x7fa7427532e0 , replication = 0x0, pguser =
0x7fa7427532c0 XX, pgpass = 0x7fa742755070 XXX, keepalives
= 0x0,
keepalives_idle = 0x0, keepalives_interval = 0x0, keepalives_count = 0x0,
sslmode = 0x7fa7427533c0 prefer, sslcompression = 0x7fa7427533e0 1, sslkey
= 0x0, sslcert = 0x0, sslrootcert = 0x0, sslcrl = 0x0, requirepeer = 0x0,
krbsrvname = 0x7fa742753400 postgres, Pfdebug = 0x0, noticeHooks =
{noticeRec = 0x7fa73fed3ec0 defaultNoticeReceiver, noticeRecArg = 0x0,
noticeProc = 0x7fa740572f20 NoticeProcessor, noticeProcArg = 0x0}, events =
0x0,
nEvents = 0, eventArraySize = 0, status = CONNECTION_BAD, asyncStatus =
PGASYNC_COPY_IN, xactStatus = PQTRANS_INTRANS, queryclass = PGQUERY_SIMPLE,
last_query = 0x7fa7427809f0 COPY XX (XX, XX,
XX, XX, XX) FROM stdin;, last_sqlstate = \000\000\000\000\000,
options_valid = 1 '\001',
nonblocking = 0 '\000', singleRowMode = 0 '\000', copy_is_binary = 0 '\000',
copy_already_done = 0, notifyHead = 0x0, notifyTail = 0x0, sock = -1, laddr =
{addr = {ss_family = 2, __ss_align = 0,
__ss_padding = '\000' repeats 111 times}, salen = 16}, raddr = {addr =
{ss_family = 2, __ss_align = 0, __ss_padding = '\000' repeats 111 times},
salen = 16}, pversion = 196608, sversion = 90302, auth_req_received = 1 '\001',
password_needed = 1 '\001', dot_pgpass_used = 1 '\001', sigpipe_so = 0
'\000', sigpipe_flag = 0 '\000', addrlist = 0x0, addr_cur = 0x0,
addrlist_family = 0, setenv_state = SETENV_STATE_IDLE, next_eo = 0x0,
send_appname = 1 '\001',
be_pid = 25857, be_key = 393001970, md5Salt = XX, pstatus =
0x7fa742788630, client_encoding = 6, std_strings = 1 '\001', verbosity =
PQERRORS_DEFAULT, lobjfuncs = 0x0, inBuffer = 0x7fa74274a740 G, inBufSize =
16384,
inStart = 0, inCursor = 0, inEnd = 0, outBuffer = 0x7fa63a495010
00229878919\t1, '0' repeats 12 times, 13462\tsupplemental\tt\nd,
outBufSize = 2147475456, outCount = 2147475445, outMsgStart = 2147475446,
outMsgEnd = 2147475450,
rowBuf = 0x7fa742752760, rowBufLen = 32, result = 0x0, next_result = 0x0,
allow_ssl_try = 1 '\001', wait_ssl_try = 0 '\000', ssl = 0x0, peer = 0x0,
engine = 0x0, gctx = 0x0, gtarg_nam = 0x0, ginbuf = {length = 0, value = 0x0},
goutbuf = {length = 0, value = 0x0}, errorMessage = {data = 0x7fa742752970
cannot allocate memory for output buffer\n, len = 41, maxlen = 256},
workBuffer = {data = 0x7fa742752a80 SAVEPOINT, len = 9, maxlen = 256}}
You can see asyncStatus is still PGASYNC_COPY_IN and also how
outBufSize grew up to the 2G mark, along with the 'cannot allocate'
error.
The p *res looked like:
$2 = {ntups = 0, numAttributes = 0, attDescs = 0x0, tuples = 0x0, tupArrSize =
0, numParameters = 0, paramDescs = 0x0, resultStatus = PGRES_COPY_IN,
cmdStatus =
\000\177\000\000@\000\000\000\000\000\000\000!\000\000\000\000\000\000\000\210ף?\247\177\000\000\000BxB\247\177\000\000`\000\000\000\000\000\000\000\060\000\000\000\000\000\000\000\300dxB\247\177\000\000\000\000\000,
binary = 0, noticeHooks = {noticeRec = 0x7fa73fed3ec0
defaultNoticeReceiver, noticeRecArg = 0x0, noticeProc = 0x7fa740572f20
NoticeProcessor, noticeProcArg = 0x0}, events = 0x0, nEvents = 0,
client_encoding = 6, errMsg = 0x0,
errFields = 0x0, null_field = , curBlock = 0x0, curOffset = 0, spaceLeft =
0}
If so, maybe we need an explicit test on the connection being good before
we return valid PGRES_COPY_IN etc results.
Right, PQresultStatus() just checks if the PGresult is non-NULL, it's
not doing any checking of the connection state itself. I had been
trying to work out if something else should have been responsible for
changing asyncStatus away from PGASYNC_COPY_IN when
Re: [HACKERS] [PERFORM] encouraging index-only scans
On Tue, Feb 11, 2014 at 05:51:36PM -0200, Claudio Freire wrote: We track relpages and relallvisible, which seems like a more direct measure. Once analyze is done (which is already triggered by inserts) and sets those, it could fire a vacuum based on the ratio of those values, or the autovac process could just look at the ratio after naptime. So just introduce autovacuum_vacuum_visible_factor. A problem there is that it would be a lot of work to aggressively keep the ratio high, and pointless if the types of queries done on that table don't benefit from IOS anyway, or if pages are dirtied so rapidly that no amount of vacuuming will keep the ratio high. Would we try to automatically tell which tables were which, or rely on the DBA setting per-table autovacuum_vacuum_visible_factor for tables that differ from the database norm? Why not track how many times an IOS would be used but wasn't, or how many heap fetches in IOS have to be performed? Seems like a more direct measure of whether allvisible needs an update. Now that is in interesting idea, and more direct. Do we need to adjust for the insert count, i.e. would the threadhold to trigger an autovacuum after finding index lookups that had to check the heap page for visibility be higher if many inserts are happening, perhaps dirtying pages? (If we are dirtying via update/delete, autovacuum will already trigger.) We are aggressive in clearing the page-all-visible flag (we have to be), but I think we need a little more aggressiveness for setting it. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] Small GIN optimizations (after 9.4)
On Sun, Feb 9, 2014 at 02:17:12PM +0400, Alexander Korotkov wrote: On Thu, Feb 6, 2014 at 8:31 PM, PostgreSQL - Hans-J rgen Sch nig postg...@cybertec.at wrote: i think there is one more thing which would be really good in GIN and which would solve a ton of issues. atm GIN entries are sorted by item pointer. if we could sort them by a column it would fix a couple of real work issues such as ... SELECT ... FROM foo WHERE tsearch_query ORDER BY price DESC LIMIT 10 ... or so. it many cases you want to search for a, say, product and find the cheapest / most expensive one. if the tsearch_query yields a high number of rows (which it often does) the subsequent sort will kill you. This is not intended to be a small change. However, some solution might be possible in post 9.4 gin improvements or in new secret indexing project which will be presented at PGCon :-) Would any of the listed changes cause backward-incompatible changes to the on-disk format, causing problems for pg_upgrade? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] GSOC13 proposal - extend RETURNING syntax
On Sun, Feb 02, 2014 at 02:52:42PM -0800, David Fetter wrote:
On Wed, Aug 21, 2013 at 08:52:25PM +0200, Karol Trzcionka wrote:
W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze:
With this fixed, a more complete review:
Thanks.
I've done some syntactic and white space cleanup, here attached.
Karol, would you care to help with commenting the sections that need
same?
Karol,
Thanks for the updates :)
Other folks,
Next version attached.
Cheers,
David.
--
David Fetter da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 90b9208..5addfc1 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -194,12 +194,27 @@ UPDATE [ ONLY ] replaceable
class=PARAMETERtable_name/replaceable [ * ] [
termreplaceable class=PARAMETERoutput_expression/replaceable/term
listitem
para
- An expression to be computed and returned by the commandUPDATE/
- command after each row is updated. The expression can use any
- column names of the table named by replaceable
class=PARAMETERtable_name/replaceable
- or table(s) listed in literalFROM/.
- Write literal*/ to return all columns.
+ An expression to be computed and returned by the
+ commandUPDATE/ command either before or after (prefixed with
+ literalBEFORE./literal and literalAFTER./literal,
+ respectively) each row is updated. The expression can use any
+ column names of the table named by replaceable
+ class=PARAMETERtable_name/replaceable or table(s) listed in
+ literalFROM/. Write literalAFTER.*/literal to return all
+ columns after the update. Write literalBEFORE.*/literal for all
+ columns before the update. Write literal*/literal to return all
+ columns after update and all triggers fired (these values are in table
+ after command). You may combine BEFORE, AFTER and raw columns in the
+ expression.
/para
+ warningpara
+ Mixing table names or aliases named before or after with the
+ above will result in confusion and suffering. If you happen to
+ have a table called literalbefore/literal or
+ literalafter/literal, alias it to something else when using
+ RETURNING.
+ /para/warning
+
/listitem
/varlistentry
@@ -287,12 +302,12 @@ UPDATE weather SET temp_lo = temp_lo+1, temp_hi =
temp_lo+15, prcp = DEFAULT
/para
para
- Perform the same operation and return the updated entries:
+ Perform the same operation and return information on the changed entries:
programlisting
UPDATE weather SET temp_lo = temp_lo+1, temp_hi = temp_lo+15, prcp = DEFAULT
WHERE city = 'San Francisco' AND date = '2003-07-03'
- RETURNING temp_lo, temp_hi, prcp;
+ RETURNING temp_lo AS new_low, temp_hi AS new_high,
BEFORE.temp_hi/BEFORE.temp_low AS old_ratio, AFTER.temp_hi/AFTER.temp_low AS
new_ratio prcp;
/programlisting
/para
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 86449a6..ad4eecb 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2335,7 +2335,7 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo
*relinfo)
TupleTableSlot *
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
-ItemPointer tupleid, TupleTableSlot
*slot)
+ItemPointer tupleid, TupleTableSlot
*slot, TupleTableSlot **planSlot)
{
TriggerDesc *trigdesc = relinfo-ri_TrigDesc;
HeapTuple slottuple = ExecMaterializeSlot(slot);
@@ -2378,10 +2378,15 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
* junkfilter's output slot, so we are clobbering the original value of
* slottuple by doing the filtering. This is OK since neither we nor
our
* caller have any more interest in the prior contents of that slot.
+*
+* Execution plan is changed so it is reported up by planSlot,
+* it is needed to get correct value for BEFORE/AFTER statements
+* in RETURNING syntax.
*/
if (newSlot != NULL)
{
slot = ExecFilterJunk(relinfo-ri_junkFilter, newSlot);
+ *planSlot = newSlot;
slottuple = ExecMaterializeSlot(slot);
newtuple = slottuple;
}
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index 6f0f47e..926a80b 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -604,12 +604,17 @@ ExecUpdate(ItemPointer tupleid,
resultRelInfo =
Re: [HACKERS] Unhappy with error handling in psql's handleCopyOut()
I wrote: Stephen Frost sfr...@snowman.net writes: I've not gotten back to it yet, but I ran into a related-seeming issue where psql would happily chew up 2G of memory trying to send COPY failed notices when it gets disconnected from a server that it's trying to send data to mid-COPY. conn-sock was -1, connection was 'CONNECTION_BAD', but the loop towards the end of handleCopyIn doesn't care and nothing in libpq is changing PQresultStatus(): [ scratches head... ] Offhand I'd have expected PQgetResult to start returning error indications. After some study of the code I have a theory about this. Note that we don't close the socket on send failures anymore; if the status is CONNECTION_BAD, that pretty much has to have been done by pqReadData. Which wouldn't be called in a PQputCopyData loop, as long as everything was going normally. Ordinarily, while we're pumping data to the backend, we'll flush libpq's output buffer every 8K (see pqPutMsgEnd). Suppose that, in one of those calls to pqSendSome, we fail to send everything in the buffer but send() doesn't report a hard error --- maybe it says EINTR, or maybe it just returns zero. Since there's still data to send, now pqSendSome will call pqReadData. Assume that the read attempt *does* give a hard error; whereupon pqReadData closes the socket, sets CONNECTION_BAD, and returns -1. pqSendSome falls out, leaving data still in the output buffer, and then we'll return -1 to handleCopyIn, which abandons its data sending loop and calls PQputCopyEnd. But there's still = 8K in the buffer, so when PQputCopyEnd calls pqPutMsgEnd, the latter tries to send it. And fails. (What's more, pqSendSome doesn't clear the output buffer when it fails at the top because the socket's gone.) This results in PQputCopyEnd exiting without having changed the asyncStatus back to ASYNC_BUSY from ASYNC_COPY_IN. Now PQgetResult will happily return a COPY_IN result, so we loop, and the whole thing repeats, with no state change except we keep adding data to the buffer. I think we need to change pqSendSome to reset outCount to zero if it exits at the top due to no socket. It would do that if it got a hard error from pqsecure_write, so why's it not doing so if there's not even a socket? This would avoid the problem of bloating the output buffer while we continue to queue data that can never be sent. (It might be that some of the other error situations in pqSendSome should also abandon unwritten data, but that's less clear.) Another thought is that maybe PQputCopyEnd should change asyncStatus even if it fails to queue the copy-end message. I think the reason why it's coded like it is is the idea that if we've not queued the message, then the backend still thinks the copy is active, so we should not change state. However, if we've already closed the socket then this is pretty much moot, so perhaps there needs to be an exception allowing the state change in that case. Also, if the output buffer is already over 8K, we're failing to distinguish couldn't queue the message from couldn't send the message; but I'm not sure it's worth refactoring to detect that case. Alternatively, we could do what I suggested earlier and change PQgetResult to not return an ordinary COPY_IN or COPY_OUT status if the connection is known dead. This seems probably more robust than tinkering at the margins in PQputCopyEnd. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] narwhal and PGDLLIMPORT
Craig Ringer cr...@2ndquadrant.com writes: On 02/11/2014 01:28 PM, Tom Lane wrote: If there are no objections, I'll push this patch into HEAD tomorrow, along with the upthread patches from Craig Ringer and Marco Atzeri. We might as well see if this stuff is going to work ... I'd love to test my patch properly before pushing it, but my dev machine is going to need a total teardown and rebuild, and I'm currently focused on polishing off urgent open work. So let's see what the buildfarm makes of it, I guess. So the early returns from currawong are interesting: d:\bf\root\HEAD\pgsql.920\pgsql.sln (default target) (1) - (contrib\pg_buffercache target) - pg_buffercache_pages.obj : error LNK2001: unresolved external symbol _MainLWLockArray .\Release\pg_buffercache\pg_buffercache.dll : fatal error LNK1120: 1 unresolved externals d:\bf\root\HEAD\pgsql.920\pgsql.sln (default target) (1) - (contrib\pg_stat_statements target) - pg_stat_statements.obj : error LNK2001: unresolved external symbol _MainLWLockArray .\Release\pg_stat_statements\pg_stat_statements.dll : fatal error LNK1120: 1 unresolved externals d:\bf\root\HEAD\pgsql.920\pgsql.sln (default target) (1) - (contrib\test_shm_mq target) - worker.obj : error LNK2001: unresolved external symbol _ProcDiePending worker.obj : error LNK2001: unresolved external symbol _proc_exit_inprogress .\Release\test_shm_mq\test_shm_mq.dll : fatal error LNK1120: 2 unresolved externals I guess this is good news in one sense: now we're getting results from an MSVC machine that are consistent with what narwhal has been telling us. But how is it that your patch caused this to be reported when it wasn't before? And how come we're not getting the results we hoped for, that these symbols would be effectively exported without needing PGDLLIMPORT? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Small GIN optimizations (after 9.4)
On Wed, Feb 12, 2014 at 2:58 AM, Bruce Momjian br...@momjian.us wrote: On Sun, Feb 9, 2014 at 02:17:12PM +0400, Alexander Korotkov wrote: On Thu, Feb 6, 2014 at 8:31 PM, PostgreSQL - Hans-J rgen Sch nig postg...@cybertec.at wrote: i think there is one more thing which would be really good in GIN and which would solve a ton of issues. atm GIN entries are sorted by item pointer. if we could sort them by a column it would fix a couple of real work issues such as ... SELECT ... FROM foo WHERE tsearch_query ORDER BY price DESC LIMIT 10 ... or so. it many cases you want to search for a, say, product and find the cheapest / most expensive one. if the tsearch_query yields a high number of rows (which it often does) the subsequent sort will kill you. This is not intended to be a small change. However, some solution might be possible in post 9.4 gin improvements or in new secret indexing project which will be presented at PGCon :-) Would any of the listed changes cause backward-incompatible changes to the on-disk format, causing problems for pg_upgrade? None of them. -- With best regards, Alexander Korotkov.
Re: [HACKERS] narwhal and PGDLLIMPORT
I wrote: Hiroshi Inoue in...@tpf.co.jp writes: I tried MINGW port with the attached change and successfully built src and contrib and all pararell regression tests were OK. I cleaned this up a bit (the if-nesting in Makefile.shlib was making my head hurt, not to mention that it left a bunch of dead code) and committed it. Hm ... according to buildfarm member narwhal, this doesn't work so well for plperl: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -Wno-comment -shared -o plperl.dll plperl.o SPI.o Util.o -L../../../src/port -L../../../src/common -Wl,--allow-multiple-definition -L/mingw/lib -Wl,--as-needed -LC:/Perl/lib/CORE -lperl58 -L../../../src/backend -lpostgres -lpgcommon -lpgport -lintl -lxslt -lxml2 -lssleay32 -leay32 -lz -lm -lws2_32 -lshfolder -Wl,--export-all-symbols -Wl,--out-implib=libplperl.a Cannot export .idata$4: symbol not found Cannot export .idata$5: symbol not found Cannot export .idata$6: symbol not found Cannot export .text: symbol not found Cannot export perl58_NULL_THUNK_DATA: symbol not found Creating library file: libplperl.a collect2: ld returned 1 exit status make[3]: *** [plperl.dll] Error 1 Not very clear what's going on there; could this be a problem in narwhal's admittedly-ancient toolchain? BTW, now that I look at this ... why are we bothering to build static libraries (.a files) for DLLs? They have no possible use AFAICS. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] narwhal and PGDLLIMPORT
On 02/12/2014 07:22 AM, Tom Lane wrote: So the early returns from currawong are interesting: d:\bf\root\HEAD\pgsql.920\pgsql.sln (default target) (1) - (contrib\pg_buffercache target) - pg_buffercache_pages.obj : error LNK2001: unresolved external symbol _MainLWLockArray .\Release\pg_buffercache\pg_buffercache.dll : fatal error LNK1120: 1 unresolved externals d:\bf\root\HEAD\pgsql.920\pgsql.sln (default target) (1) - (contrib\pg_stat_statements target) - pg_stat_statements.obj : error LNK2001: unresolved external symbol _MainLWLockArray .\Release\pg_stat_statements\pg_stat_statements.dll : fatal error LNK1120: 1 unresolved externals d:\bf\root\HEAD\pgsql.920\pgsql.sln (default target) (1) - (contrib\test_shm_mq target) - worker.obj : error LNK2001: unresolved external symbol _ProcDiePending worker.obj : error LNK2001: unresolved external symbol _proc_exit_inprogress .\Release\test_shm_mq\test_shm_mq.dll : fatal error LNK1120: 2 unresolved externals Great, that's what I was hoping to see - proper errors where we've omitted things, not silent miscompilation. I guess this is good news in one sense: now we're getting results from an MSVC machine that are consistent with what narwhal has been telling us. But how is it that your patch caused this to be reported when it wasn't before? And how come we're not getting the results we hoped for, that these symbols would be effectively exported without needing PGDLLIMPORT? Unfortunately I don't think we can get to totally invisible with the MS toolchain. We're still going to have to annotate exported symbols, but at least we can tell where code tries to import something that hasn't been annotated. This just fixes the bug in the .def generator that was permitting wrong code to compile silently and incorrectly, rather than fail to link. It makes omitted PGDLLIMPORT's obvious, but it doesn't appear to be possible to tell the toolchain that the *importing side* should auto-detect whether an extern is from another DLL and automatically __declspec(dllimport) it. As I understand it, what's happening here is that the importing side (the contrib/tool) is trying to do old-style DLL linkage, where the export library for the DLL is expected to expose a symbol referring to *a pointer to the real data* that gets statically linked into the importing side. We're trying to link to those pointer symbols and are failing because the `DATA` annotation suppresses their generation. (Remember that in Microsoft-land you don't actually link to a .DLL at compilation time, you statically link to the export library .LIB for the DLL, which contains stubs and fixup information). If we instead emitted CONST in the .DEF file, these indirect pointers would be generated by the linker and added to the export library, and we'd link to those instead. Our code doesn't expect the extra level of indirection introduced and would fail (mostly) at runtime, much like what was happening when we were linking to function stubs. The only way I can see to actually get unix-like linkage is to use __declspec(dllimport) on the *importing side*, e.g. when compiling contribs etc. That means we still need the much-hated windows-droppings, but we can at least see when they got left out. (On a side note, I'm starting to suspect - and need to verify - that as a result of not using PGDLLIMPORT on exported functions we're actually doing old-style thunk function calls from extensions into postgres.exe, not direct function calls, for a similar reason.) I'm going to go wash my brain out now, it feels dirty. -- Craig Ringer 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] narwhal and PGDLLIMPORT
On 02/11/2014 11:04 PM, Amit Kapila wrote: On Tue, Feb 11, 2014 at 11:01 AM, Craig Ringer cr...@2ndquadrant.com wrote: On 02/11/2014 01:28 PM, Tom Lane wrote: If there are no objections, I'll push this patch into HEAD tomorrow, along with the upthread patches from Craig Ringer and Marco Atzeri. We might as well see if this stuff is going to work ... I'd love to test my patch properly before pushing it, but my dev machine is going to need a total teardown and rebuild, I can do the test of your patch/idea, please confirm if below steps are sufficient: a. Change manually postgres.def file and add DATA for MainLWLockArray. (Will it be sufficient to change manually or should I apply your patch) No, you must rebuild postgres, or at least re-generate the .DEF file and re-link postgres.exe to generate a new import library (.lib) for it. It'll be easiest to just recompile. b. Rebuild pg_buffercache module Yes. c. Test pg_buffercache if it can access the variable. Yes, though it should fail to link if there's no PGDLLIMPORT. d. If above works, then run 'check'. Yes, after adding the PGDLLIMPORT and re-generating the postgres .lib by relinking or recompiling. If I understand correctly your patch is intended to resolve PGDLLIMPORT problem, right? No, just make it obvious where such imports are missing. I can't see a way to completely make it invisible with the MS toolchain. -- Craig Ringer 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] narwhal and PGDLLIMPORT
On 02/12/2014 07:30 AM, Tom Lane wrote: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -Wno-comment -shared -o plperl.dll plperl.o SPI.o Util.o -L../../../src/port -L../../../src/common -Wl,--allow-multiple-definition -L/mingw/lib -Wl,--as-needed -LC:/Perl/lib/CORE -lperl58 -L../../../src/backend -lpostgres -lpgcommon -lpgport -lintl -lxslt -lxml2 -lssleay32 -leay32 -lz -lm -lws2_32 -lshfolder -Wl,--export-all-symbols -Wl,--out-implib=libplperl.a Cannot export .idata$4: symbol not found Cannot export .idata$5: symbol not found Cannot export .idata$6: symbol not found Cannot export .text: symbol not found Cannot export perl58_NULL_THUNK_DATA: symbol not found Creating library file: libplperl.a collect2: ld returned 1 exit status make[3]: *** [plperl.dll] Error 1 Not very clear what's going on there; could this be a problem in narwhal's admittedly-ancient toolchain? Easily. BTW, now that I look at this ... why are we bothering to build static libraries (.a files) for DLLs? They have no possible use AFAICS. This is actually compiling a DLL: -o plperl.dll but is also emitting an import library: -Wl,--out-implib=libplperl.a which is required if you wish to link to plperl.dll from any other compilation unit (except for by dynamic linkage). I don't see any use for that with plperl, but it might be a valid thing to be doing for (e.g.) hstore.dll. Though you can't really link to it from another module anyway, you have to go through the fmgr to get access to its symbols at rutime, so we can probably just skip generation of import libraries for contribs and PLs. -- Craig Ringer 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] narwhal and PGDLLIMPORT
Craig Ringer cr...@2ndquadrant.com writes: On 02/12/2014 07:30 AM, Tom Lane wrote: BTW, now that I look at this ... why are we bothering to build static libraries (.a files) for DLLs? They have no possible use AFAICS. I don't see any use for that with plperl, but it might be a valid thing to be doing for (e.g.) hstore.dll. Though you can't really link to it from another module anyway, you have to go through the fmgr to get access to its symbols at rutime, so we can probably just skip generation of import libraries for contribs and PLs. On second thought, I believe we need it for, say, libpq. Not sure if it's worth adding the Makefile logic that'd be needed to not build the import library for other cases. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] narwhal and PGDLLIMPORT
Craig Ringer cr...@2ndquadrant.com writes: On 02/12/2014 07:22 AM, Tom Lane wrote: So the early returns from currawong are interesting: Great, that's what I was hoping to see - proper errors where we've omitted things, not silent miscompilation. Well, before you get too optimistic about that benighted platform ... mastodon just reported in on this patch, and it's showing a *different* set of unresolved symbols than currawong is. None of them are a surprise exactly, but nonetheless, why didn't currawong find the postgres_fdw issues? It still seems there's something unexplained here. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] narwhal and PGDLLIMPORT
On 02/12/2014 08:30 AM, Tom Lane wrote: Craig Ringer cr...@2ndquadrant.com writes: On 02/12/2014 07:22 AM, Tom Lane wrote: So the early returns from currawong are interesting: Great, that's what I was hoping to see - proper errors where we've omitted things, not silent miscompilation. Well, before you get too optimistic about that benighted platform ... mastodon just reported in on this patch, and it's showing a *different* set of unresolved symbols than currawong is. None of them are a surprise exactly, but nonetheless, why didn't currawong find the postgres_fdw issues? It still seems there's something unexplained here. Looks like currawong doesn't build postgres_fdw. -- Craig Ringer 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] narwhal and PGDLLIMPORT
On 02/11/2014 08:04 PM, Craig Ringer wrote: On 02/12/2014 08:30 AM, Tom Lane wrote: Craig Ringer cr...@2ndquadrant.com writes: On 02/12/2014 07:22 AM, Tom Lane wrote: So the early returns from currawong are interesting: Great, that's what I was hoping to see - proper errors where we've omitted things, not silent miscompilation. Well, before you get too optimistic about that benighted platform ... mastodon just reported in on this patch, and it's showing a *different* set of unresolved symbols than currawong is. None of them are a surprise exactly, but nonetheless, why didn't currawong find the postgres_fdw issues? It still seems there's something unexplained here. Looks like currawong doesn't build postgres_fdw. It sure used to: http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=currawongdt=2014-02-03%2005%3A30%3A00stg=make cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unhappy with error handling in psql's handleCopyOut()
I wrote:
Stephen Frost sfr...@snowman.net writes:
I've not gotten back to it yet, but I ran into a related-seeming issue
where psql would happily chew up 2G of memory trying to send COPY
failed notices when it gets disconnected from a server that it's trying
to send data to mid-COPY. conn-sock was -1, connection was
'CONNECTION_BAD', but the loop towards the end of handleCopyIn doesn't
care and nothing in libpq is changing PQresultStatus():
After some study of the code I have a theory about this.
I was able to reproduce this misbehavior by setting a gdb breakpoint
at pqReadData and then killing the connected server process while psql's
COPY IN was stopped there. Resetting outCount to zero in the
socket-already-gone case in pqSendSome is enough to fix the problem.
However, I think it's also prudent to hack PQgetResult so that it
won't return a copy in progress status if the connection is known
dead.
The error recovery behavior in pqSendSome has been like this since 8.1
or thereabouts, so I'm inclined to push something like the attached into
all branches.
regards, tom lane
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 5f371b4..764e90c 100644
*** a/src/interfaces/libpq/fe-exec.c
--- b/src/interfaces/libpq/fe-exec.c
*** static int PQsendQueryGuts(PGconn *conn,
*** 63,68
--- 63,69
const int *paramFormats,
int resultFormat);
static void parseInput(PGconn *conn);
+ static PGresult *getCopyResult(PGconn *conn, ExecStatusType copytype);
static bool PQexecStart(PGconn *conn);
static PGresult *PQexecFinish(PGconn *conn);
static int PQsendDescribe(PGconn *conn, char desc_type,
*** PQgetResult(PGconn *conn)
*** 1734,1755
conn-asyncStatus = PGASYNC_BUSY;
break;
case PGASYNC_COPY_IN:
! if (conn-result conn-result-resultStatus == PGRES_COPY_IN)
! res = pqPrepareAsyncResult(conn);
! else
! res = PQmakeEmptyPGresult(conn, PGRES_COPY_IN);
break;
case PGASYNC_COPY_OUT:
! if (conn-result conn-result-resultStatus == PGRES_COPY_OUT)
! res = pqPrepareAsyncResult(conn);
! else
! res = PQmakeEmptyPGresult(conn, PGRES_COPY_OUT);
break;
case PGASYNC_COPY_BOTH:
! if (conn-result conn-result-resultStatus == PGRES_COPY_BOTH)
! res = pqPrepareAsyncResult(conn);
! else
! res = PQmakeEmptyPGresult(conn, PGRES_COPY_BOTH);
break;
default:
printfPQExpBuffer(conn-errorMessage,
--- 1735,1747
conn-asyncStatus = PGASYNC_BUSY;
break;
case PGASYNC_COPY_IN:
! res = getCopyResult(conn, PGRES_COPY_IN);
break;
case PGASYNC_COPY_OUT:
! res = getCopyResult(conn, PGRES_COPY_OUT);
break;
case PGASYNC_COPY_BOTH:
! res = getCopyResult(conn, PGRES_COPY_BOTH);
break;
default:
printfPQExpBuffer(conn-errorMessage,
*** PQgetResult(PGconn *conn)
*** 1786,1791
--- 1778,1813
return res;
}
+ /*
+ * getCopyResult
+ * Helper for PQgetResult: generate result for COPY-in-progress cases
+ */
+ static PGresult *
+ getCopyResult(PGconn *conn, ExecStatusType copytype)
+ {
+ /*
+ * If the server connection has been lost, don't pretend everything is
+ * hunky-dory; instead return a PGRES_FATAL_ERROR result, and reset the
+ * asyncStatus to idle (corresponding to what we'd do if we'd detected I/O
+ * error in the earlier steps in PQgetResult). The text returned in the
+ * result is whatever is in conn-errorMessage; we expect that was filled
+ * with something useful when the lost connection was detected.
+ */
+ if (conn-status != CONNECTION_OK)
+ {
+ pqSaveErrorResult(conn);
+ conn-asyncStatus = PGASYNC_IDLE;
+ return pqPrepareAsyncResult(conn);
+ }
+
+ /* If we have an async result for the COPY, return that */
+ if (conn-result conn-result-resultStatus == copytype)
+ return pqPrepareAsyncResult(conn);
+
+ /* Otherwise, invent a suitable PGresult */
+ return PQmakeEmptyPGresult(conn, copytype);
+ }
+
/*
* PQexec
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 1eb3ac6..a7afd42 100644
*** a/src/interfaces/libpq/fe-misc.c
--- b/src/interfaces/libpq/fe-misc.c
*** pqSendSome(PGconn *conn, int len)
*** 804,809
--- 804,811
{
printfPQExpBuffer(conn-errorMessage,
libpq_gettext(connection not open\n));
+ /* Discard queued data; no chance it'll ever be sent */
+ conn-outCount = 0;
return -1;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] narwhal and PGDLLIMPORT
Andrew Dunstan and...@dunslane.net writes: On 02/11/2014 08:04 PM, Craig Ringer wrote: Looks like currawong doesn't build postgres_fdw. It sure used to: http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=currawongdt=2014-02-03%2005%3A30%3A00stg=make Hm, does the MSVC build system do parallel builds? Maybe it abandoned the build after noticing the first failure, and whether you get to see more than one is timing-dependent. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] truncating pg_multixact/members
On Tue, Feb 11, 2014 at 5:16 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Robert Haas escribió: On Mon, Jan 20, 2014 at 1:39 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: * I haven't introduced settings to tweak this per table for autovacuum. I don't think those are needed. It's not hard to do, however; if people opine against this, I will implement that. I can't think of any reason to believe that it will be less important to tune these values on a per-table basis than it is to be able to do the same with the autovacuum parameters. Indeed, all the discussion on this thread suggests precisely that we have no real idea how to set these values yet, so more configurability is good. Even if you reject that argument, I think it's a bad idea to start making xmax vacuuming and xmin vacuuming less than parallel; such decisions confuse users. Yeah, I can relate to this argument. I have added per-table configurability to this, and also added the an equivalent of autovacuum_freeze_max_age to force a for-wraparound full scan of a table based on multixacts. I haven't really tested this beyond ensuring that it compiles, and I haven't changed the default values, but here it is in case someone wants to have a look and comment --- particularly on the doc additions. Using Multixact capitalized just so seems odd to me. Probably should be lower case (multiple places). This part needs some copy-editing: + para +Vacuum also allows removal of old files from the +filenamepg_multixact/members/ and filenamepg_multixact/offsets/ +subdirectories, which is why the default is a relatively low +50 million transactions. Vacuuming multixacts also allows...? And: 50 million multixacts, not transactions. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unhappy with error handling in psql's handleCopyOut()
* Tom Lane (t...@sss.pgh.pa.us) wrote: I was able to reproduce this misbehavior by setting a gdb breakpoint at pqReadData and then killing the connected server process while psql's COPY IN was stopped there. Resetting outCount to zero in the socket-already-gone case in pqSendSome is enough to fix the problem. However, I think it's also prudent to hack PQgetResult so that it won't return a copy in progress status if the connection is known dead. Agreed. The error recovery behavior in pqSendSome has been like this since 8.1 or thereabouts, so I'm inclined to push something like the attached into all branches. Looks good to me, thanks for running this down! Stephen signature.asc Description: Digital signature
Re: [HACKERS] improve the help message about psql -F
If you are going to change the help string for -F, you should also update the help string for -R, and possibly for -z and -0. -- 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] narwhal and PGDLLIMPORT
On 02/12/2014 09:45 AM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 02/11/2014 08:04 PM, Craig Ringer wrote: Looks like currawong doesn't build postgres_fdw. It sure used to: http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=currawongdt=2014-02-03%2005%3A30%3A00stg=make Hm, does the MSVC build system do parallel builds? Maybe it abandoned the build after noticing the first failure, and whether you get to see more than one is timing-dependent. I'm pretty certain it does, and that's quite a reasonable explanation. That reminds me - in addition to the option to force a serial build, I really need to post a patch for the msvc build that lets you suppress VERBOSE mode. -- Craig Ringer 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] narwhal and PGDLLIMPORT
(2014/02/12 8:30), Tom Lane wrote: I wrote: Hiroshi Inoue in...@tpf.co.jp writes: I tried MINGW port with the attached change and successfully built src and contrib and all pararell regression tests were OK. I cleaned this up a bit (the if-nesting in Makefile.shlib was making my head hurt, not to mention that it left a bunch of dead code) and committed it. Hm ... according to buildfarm member narwhal, this doesn't work so well for plperl: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -Wno-comment -shared -o plperl.dll plperl.o SPI.o Util.o -L../../../src/port -L../../../src/common -Wl,--allow-multiple-definition -L/mingw/lib -Wl,--as-needed -LC:/Perl/lib/CORE -lperl58 -L../../../src/backend -lpostgres -lpgcommon -lpgport -lintl -lxslt -lxml2 -lssleay32 -leay32 -lz -lm -lws2_32 -lshfolder -Wl,--export-all-symbols -Wl,--out-implib=libplperl.a Cannot export .idata$4: symbol not found Cannot export .idata$5: symbol not found Cannot export .idata$6: symbol not found Cannot export .text: symbol not found Cannot export perl58_NULL_THUNK_DATA: symbol not found Creating library file: libplperl.a collect2: ld returned 1 exit status make[3]: *** [plperl.dll] Error 1 Oops I forgot to inclule plperl, tcl or python, sorry. I would retry build operations with them. Unfortunately it would take pretty long time because build operations are pretty (or veeery in an old machine) slow. Not very clear what's going on there; could this be a problem in narwhal's admittedly-ancient toolchain? BTW, now that I look at this ... why are we bothering to build static libraries (.a files) for DLLs? They have no possible use AFAICS. They are import libraries though I doubt such plugins need them. regards, Hiroshi Inoue -- I am using the free version of SPAMfighter. SPAMfighter has removed 3592 of my spam emails to date. Get the free SPAMfighter here: http://www.spamfighter.com/len Do you have a slow PC? Try a Free scan http://www.spamfighter.com/SLOW-PCfighter?cid=sigen -- 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] Performance Improvement by reducing WAL for Update Operation
On Tue, Feb 11, 2014 at 10:07 PM, Bruce Momjian br...@momjian.us wrote: On Wed, Feb 5, 2014 at 10:57:57AM -0800, Peter Geoghegan wrote: On Wed, Feb 5, 2014 at 12:50 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: I think there's zero overlap. They're completely complimentary features. It's not like normal WAL records have an irrelevant volume. Correct. Compressing a full-page image happens on the first update after a checkpoint, and the diff between old and new tuple is not used in that case. Uh, I really just meant that one thing that might overlap is considerations around the choice of compression algorithm. I think that there was some useful discussion of that on the other thread as well. Yes, that was my point. I though the compression of full-page images was a huge win and that compression was pretty straight-forward, except for the compression algorithm. If the compression algorithm issue is resolved, By issue, I assume you mean to say, which compression algorithm is best for this patch. For this patch, currently we have 2 algorithm's for which results have been posted. As far as I understand Heikki is pretty sure that the latest algorithm (compression using prefix-suffix match in old and new tuple) used for this patch is better than the other algorithm in terms of CPU gain or overhead. The performance data taken by me for the worst case for this algorithm shows there is a CPU overhead for this algorithm as well. OTOH the another algorithm (compression using old tuple as history) can be a bigger win in terms I/O reduction in more number of cases. In short, it is still not decided which algorithm to choose and whether it can be enabled by default or it is better to have table level switch to enable/disable it. So I think the decision to be taken here is about below points: 1. Are we okay with I/O reduction at the expense of CPU for *worst* cases and I/O reduction without impacting CPU (better overall tps) for *favourable* cases? 2. If we are not okay with worst case behaviour, then can we provide a table-level switch, so that it can be decided by user? 3. If none of above, then is there any other way to mitigate the worst case behaviour or shall we just reject this patch and move on. Given a choice to me, I would like to go with option-2, because I think for most cases UPDATE statement will have same data for old and new tuples except for some part of tuple (generally column's having large text data are not modified), so we will be end up mostly in favourable cases and surely for worst cases we don't want user to suffer from CPU overhead, so a table-level switch is also required. I think here one might argue that for some users it is not feasible to decide whether their tuples data for UPDATE is going to be similar or completely different and they are not at all ready for any risk for CPU overhead, but they would be happy to see I/O reduction in which case it is difficult to decide what should be the value of table-level switch. Here I think the only answer is nothing is free in this world, so either make sure about the application's behaviour for UPDATE statement before going to production or just don't enable this switch and be happy with the current behaviour. On the other side there will be users who will be pretty certain about their usage of UPDATE statement or atleast are ready to evaluate their application if they can get such a huge gain, so it would be quite useful feature for such users. can we move move forward with the full-page compression patch? In my opinion, it is not certain that whatever compression algorithm got decided for this patch (if any) can be directly used for full-page compression, some ideas could be used or may be the algorithm could be tweaked a bit to make it usable for full-page compression. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] narwhal and PGDLLIMPORT
On Wed, Feb 12, 2014 at 5:30 AM, Craig Ringer cr...@2ndquadrant.com wrote: On 02/11/2014 11:04 PM, Amit Kapila wrote: On Tue, Feb 11, 2014 at 11:01 AM, Craig Ringer cr...@2ndquadrant.com wrote: On 02/11/2014 01:28 PM, Tom Lane wrote: If there are no objections, I'll push this patch into HEAD tomorrow, along with the upthread patches from Craig Ringer and Marco Atzeri. We might as well see if this stuff is going to work ... I'd love to test my patch properly before pushing it, but my dev machine is going to need a total teardown and rebuild, I can do the test of your patch/idea, please confirm if below steps are sufficient: a. Change manually postgres.def file and add DATA for MainLWLockArray. (Will it be sufficient to change manually or should I apply your patch) No, you must rebuild postgres, or at least re-generate the .DEF file and re-link postgres.exe to generate a new import library (.lib) for it. Thanks, I think already we have results from build farm, so it might not be of much use to do any additional verification/test. No, just make it obvious where such imports are missing. I can't see a way to completely make it invisible with the MS toolchain. Yes, I have also studied and tried a couple of things, but couldn't find a way to make it happen. So I think this might be the way things work for Windows platform and infact I have always used it in same way (__declspec (dllimport)) whenever there is any need. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] narwhal and PGDLLIMPORT
On Feb 12, 2014 4:09 AM, Craig Ringer cr...@2ndquadrant.com wrote: On 02/12/2014 09:45 AM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 02/11/2014 08:04 PM, Craig Ringer wrote: Looks like currawong doesn't build postgres_fdw. It sure used to: http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=currawongdt=2014-02-03%2005%3A30%3A00stg=make Hm, does the MSVC build system do parallel builds? Maybe it abandoned the build after noticing the first failure, and whether you get to see more than one is timing-dependent. I'm pretty certain it does, and that's quite a reasonable explanation. It definitely does. It does it both between individual c files in a project, and between projects (taking care about dependencies). And yes, I've seen that cause it to build modules, particularly smaller ones, in different order at different invocations based on small differences in timing. And yes, it aborts on first failure. So it would surprise me if that is *not* the problem... That reminds me - in addition to the option to force a serial build, I really need to post a patch for the msvc build that lets you suppress VERBOSE mode. That probably wouldn't hurt :-) /Magnus
Re: contrib/cache_scan (Re: [HACKERS] What's needed for cache-only table scan?)
On Sat, Feb 8, 2014 at 1:09 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: Hello, Because of time pressure in the commit-fest:Jan, I tried to simplifies the patch for cache-only scan into three portions; (1) add a hook on heap_page_prune for cache invalidation on vacuuming a particular page. (2) add a check to accept InvalidBuffer on SetHintBits (3) a proof-of-concept module of cache-only scan. (1) pgsql-v9.4-heap_page_prune_hook.v1.patch Once on-memory columnar cache is constructed, then it needs to be invalidated if heap page on behalf of the cache is modified. In usual DML cases, extension can get control using row-level trigger functions for invalidation, however, we right now have no way to get control on a page is vacuumed, usually handled by autovacuum process. This patch adds a callback on heap_page_prune(), to allow extensions to prune dead entries on its cache, not only heap pages. I'd also like to see any other scenario we need to invalidate columnar cache entries, if exist. It seems to me object_access_hook makes sense to conver DDL and VACUUM FULL scenario... (2) pgsql-v9.4-HeapTupleSatisfies-accepts-InvalidBuffer.v1.patch In case when we want to check visibility of the tuples on cache entries (thus no particular shared buffer is associated) using HeapTupleSatisfiesVisibility, it internally tries to update hint bits of tuples. However, it does not make sense onto the tuples being not associated with a particular shared buffer. Due to its definition, tuple entries being on cache does not connected with a particular shared buffer. If we need to load whole of the buffer page to set hint bits, it is totally nonsense because the purpose of on-memory cache is to reduce disk accesses. This patch adds an exceptional condition on SetHintBits() to skip anything if the given buffer is InvalidBuffer. It allows to check tuple visibility using regular visibility check functions, without re-invention of the wheel by themselves. (3) pgsql-v9.4-contrib-cache-scan.v1.patch Unlike (1) and (2), this patch is just a proof of the concept to implement cache- only scan on top of the custom-scan interface. It tries to offer an alternative scan path on the table with row-level triggers for cache invalidation if total width of referenced columns are less than 30% of the total width of table definition. Thus, it can keep larger number of records with meaningful portion on the main memory. This cache shall be invalidated according to the main heap update. One is row-level trigger, second is object_access_hook on DDL, and the third is heap_page_prune hook. Once a columns reduced tuple gets cached, it is copied to the cache memory from the shared buffer, so it needs a feature to ignore InvalidBuffer for visibility check functions. I reviewed all the three patches. The first 1 and 2 core PostgreSQL patches are fine. And I have comments in the third patch related to cache scan. 1. +# contrib/dbcache/Makefile Makefile header comment is not matched with file name location. 2.+ /* + * Estimation of average width of cached tuples - it does not make + * sense to construct a new cache if its average width is more than + * 30% of the raw data. + */ Move the estimation of average width calculation of cached tuples into the case where the cache is not found, otherwise it is an overhead for cache hit scenario. 3. + if (old_cache) + attrs_used = bms_union(attrs_used, old_cache-attrs_used); can't we need the check to see the average width is more than 30%? During estimation it doesn't include the existing other attributes. 4. + lchunk-right = cchunk; + lchunk-l_depth = TTREE_DEPTH(lchunk-right); I think it should be lchunk-r_depth needs to be set in a clock wise rotation. 5. can you add some comments in the code with how the block is used? 6. In do_insert_tuple function I felt moving the tuples and rearranging their addresses is little bit costly. How about the following way? Always insert the tuple from the bottom of the block where the empty space is started and store their corresponding reference pointers in the starting of the block in an array. As and when the new tuple inserts this array increases from block start and tuples from block end. Just need to sort this array based on item pointers, no need to update their reference pointers. In this case the movement is required only when the tuple is moved from one block to another block and also whenever if the continuous free space is not available to insert the new tuple. you can decide based on how frequent the sorting will happen in general. 7. In ccache_find_tuple function this Assert(i_min + 1 cchunk-ntups); can go wrong when only one tuple present in the block with the equal item pointer what we are searching in the forward scan direction. 8. I am not able to find a protection mechanism in insert/delete and etc of a tuple in Ttree. As this is a shared
Re: [HACKERS] narwhal and PGDLLIMPORT
(2014/02/12 3:03), Tom Lane wrote: Hiroshi Inoue in...@tpf.co.jp writes: (2014/02/09 8:06), Andrew Dunstan wrote: Yeah. Incidentally, we didn't quite get rid of dlltool for Cygwin. We did get rid of dllwrap. But I agree this is worth trying for Mingw. I tried MINGW port with the attached change and successfully built src and contrib and all pararell regression tests were OK. I cleaned this up a bit (the if-nesting in Makefile.shlib was making my head hurt, not to mention that it left a bunch of dead code) and committed it. Thanks. By my count, the only remaining usage of dlltool is in plpython's Makefile. Can we get rid of that? Maybe this is one of the few use cases of dlltool. Because python doesn't ship with its MINGW import library, the Makefile uses dlltool to generate an import library from the python DLL. Also, the only remaining usage of dllwrap is in src/bin/pgevent/Makefile. Do we need that either? Maybe this can be removed. I would make a patch later. regards, Hiroshi Inoue -- I am using the free version of SPAMfighter. SPAMfighter has removed 3597 of my spam emails to date. Get the free SPAMfighter here: http://www.spamfighter.com/len Do you have a slow PC? Try a Free scan http://www.spamfighter.com/SLOW-PCfighter?cid=sigen -- 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 min and max execute statement time in pg_stat_statement
Hi Rajeev,
(2014/01/29 17:31), Rajeev rastogi wrote:
No Issue, you can share me the test cases, I will take the performance report.
Attached patch is supported to latest pg_stat_statements. It includes min, max,
and stdev statistics. Could you run compiling test on your windows enviroments?
I think compiling error was fixed.
We had disscuttion about which is needed useful statistics in community, I think
both of statistics have storong and weak point. When we see the less(2 or 3)
executed statement, stdev will be meaningless because it cannot calculate
estimated value precisely very much, however in this situation, min and max will
be propety work well because it isn't estimated value but fact value. On the
other hand, when we see the more frequency executed statement, they will be
contrary position
statistics, stdev will be very useful statistics for estimating whole statements,
and min and max might be extremely value.
At the end of the day, these value were needed each other for more useful
statistics when we want to see several actual statments. And past my experience
showed no performance problems in this patch. So I'd like to implements all these
values in pg_stat_statements.
Regards,
--
Mitsumasa KONDO
NTT Open Source Software Center
*** a/contrib/pg_stat_statements/pg_stat_statements--1.1--1.2.sql
--- b/contrib/pg_stat_statements/pg_stat_statements--1.1--1.2.sql
***
*** 19,24 CREATE FUNCTION pg_stat_statements(IN showtext boolean,
--- 19,27
OUT query text,
OUT calls int8,
OUT total_time float8,
+ OUT min_time float8,
+ OUT max_time float8,
+ OUT stdev_time float8,
OUT rows int8,
OUT shared_blks_hit int8,
OUT shared_blks_read int8,
***
*** 41,43 CREATE VIEW pg_stat_statements AS
--- 44,51
SELECT * FROM pg_stat_statements(true);
GRANT SELECT ON pg_stat_statements TO PUBLIC;
+
+ CREATE FUNCTION pg_stat_statements_reset_time()
+ RETURNS void
+ AS 'MODULE_PATHNAME'
+ LANGUAGE C;
*** a/contrib/pg_stat_statements/pg_stat_statements--1.2.sql
--- b/contrib/pg_stat_statements/pg_stat_statements--1.2.sql
***
*** 9,14 RETURNS void
--- 9,19
AS 'MODULE_PATHNAME'
LANGUAGE C;
+ CREATE FUNCTION pg_stat_statements_reset_time()
+ RETURNS void
+ AS 'MODULE_PATHNAME'
+ LANGUAGE C;
+
CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT userid oid,
OUT dbid oid,
***
*** 16,21 CREATE FUNCTION pg_stat_statements(IN showtext boolean,
--- 21,29
OUT query text,
OUT calls int8,
OUT total_time float8,
+ OUT min_time float8,
+ OUT max_time float8,
+ OUT stdev_time float8,
OUT rows int8,
OUT shared_blks_hit int8,
OUT shared_blks_read int8,
***
*** 42,44 GRANT SELECT ON pg_stat_statements TO PUBLIC;
--- 50,53
-- Don't want this to be available to non-superusers.
REVOKE ALL ON FUNCTION pg_stat_statements_reset() FROM PUBLIC;
+ REVOKE ALL ON FUNCTION pg_stat_statements_reset_time() FROM PUBLIC;
*** a/contrib/pg_stat_statements/pg_stat_statements--unpackaged--1.0.sql
--- b/contrib/pg_stat_statements/pg_stat_statements--unpackaged--1.0.sql
***
*** 4,8
--- 4,9
\echo Use CREATE EXTENSION pg_stat_statements to load this file. \quit
ALTER EXTENSION pg_stat_statements ADD function pg_stat_statements_reset();
+ ALTER EXTENSION pg_stat_statements ADD function pg_stat_statements_reset_time();
ALTER EXTENSION pg_stat_statements ADD function pg_stat_statements();
ALTER EXTENSION pg_stat_statements ADD view pg_stat_statements;
*** a/contrib/pg_stat_statements/pg_stat_statements.c
--- b/contrib/pg_stat_statements/pg_stat_statements.c
***
*** 106,111 static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
--- 106,113
#define USAGE_DECREASE_FACTOR (0.99) /* decreased every entry_dealloc */
#define STICKY_DECREASE_FACTOR (0.50) /* factor for sticky entries */
#define USAGE_DEALLOC_PERCENT 5 /* free this % of entries at once */
+ #define EXEC_TIME_INIT_MIN DBL_MAX /* initial execution min time */
+ #define EXEC_TIME_INIT_MAX -DBL_MAX /* initial execution max time */
#define JUMBLE_SIZE1024 /* query serialization buffer size */
***
*** 137,142 typedef struct Counters
--- 139,147
{
int64 calls; /* # of times executed */
double total_time; /* total execution time, in msec */
+ double total_sqtime; /* cumulated square execution time, in msec */
+ double min_time; /* maximum execution time, in msec */
+ double max_time; /* minimum execution time, in msec */
int64 rows; /* total # of retrieved or affected rows */
int64 shared_blks_hit; /* # of shared buffer hits */
int64 shared_blks_read; /* # of shared disk blocks read */
***
*** 274,283 void _PG_init(void);
--- 279,290
void _PG_fini(void);
Datum
