Re: [HACKERS] narwhal and PGDLLIMPORT
(2014/02/15 11:42), Tom Lane wrote: Hiroshi Inoue in...@tpf.co.jp writes: (2014/02/15 2:32), Tom Lane wrote: (BTW, narwhal is evidently not trying to build plpython. I wonder why not?) Due to *initializer element is not constant* error which also can be see on my old machine. Hmm, isn't that one of the symptoms of inadequacies in --enable-auto-import? Wonder if the disable change fixed it. The error is a compilation error not a linkage one. gcc on narwhal or my old machine is too old unfortunately. regards, Hiroshi Inoue -- 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] Ctrl+C from sh can shut down daemonized PostgreSQL cluster
On 02/15/2014 02:25 AM, Greg Stark wrote: On 14 Feb 2014 23:07, Tom Lane t...@sss.pgh.pa.us mailto:t...@sss.pgh.pa.us wrote: If this is, as it sounds to be, a Solaris shell bug, doesn't it affect other daemons too? This is simmering i never exactly followed but i think if the shell doesn't support job control it's expected behaviour, not a bug. Only shells that support job control create new process groups for every backgrounded command. I would have expected if I run postgres myself that it be attached to the terminal and die when I C-c it but if it's started by pg_ctl I would have thought it was running independently of my terminal and shell. In this case maybe it is pg_ctl which should do the deamoinizing ? Cheers -- Hannu Krosing PostgreSQL Consultant Performance, Scalability and High Availability 2ndQuadrant Nordic OÜ
Re: [HACKERS] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 2014-02-15 04:20:17 +0100, Florian Pflug wrote: Another idea would be to do as you suggest and only mark the PGPROC pointers volatile, but to additionally add a check for queue corruption somewhere. We should be able to detect that - if we ever hit this issue, LWLockRelease should find a PGPROC while traversing the queue whose lwWaitLink is NULL but which isn't equal to lock-tail. If that ever happens, we'd simply PANIC. My current conclusion is that backporting barriers.h is by far the most reasonable way to go. The compiler problems have been ironed out by now... Arguments against? 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 2014-02-14 23:03:43 -0500, Robert Haas wrote: On Wed, Feb 12, 2014 at 8:00 AM, Christian Kruse christ...@2ndquadrant.com wrote: On Wednesday 12 February 2014 11:14:56 Andres Freund wrote: But they do take up shared memory without really needing to. I personally don't find that too bad, it's not much memory. If we want to avoid it we could have a LocalPgBackendStatus that includes the normal PgBackendStatus. Since pgstat_read_current_status() copies all the data locally, that'd be a sensible point to fill it. While that will cause a bit of churn, I'd guess we can use the infrastructure in the not too far away future for other parts. That's a good idea. Attached you will find a patch implementing it that way; is this how you pictured it? Although I'm not sure if this shouldn't be done in two patches, one for the changes needed for LocalPgBackendStatus and one for the xid/xmin changes. Well, this version of the patch reveals a mighty interesting point: a lot of the people who are calling pgstat_fetch_stat_beentry() don't need this additional information and might prefer not to pay the cost of fetching it. None of [functions] actually need this new information; it's only ever used in one place. So it seems like it might be wise to have pgstat_fetch_stat_beentry continue to return the PgBackendStatus * and add a new function pgstat_fetch_stat_local_beentry to fetch the LocalPgBackendStatus *; then most of these call sites wouldn't need to change. Hm maybe, seems to be as advantageous as not. Less lines changed, but a essentially duplicate function present. It would still be the case that pgstat_read_current_status() pays the price of fetching this information even if pg_stat_get_activity is never called. But since that's probably by far the most commonly-used API for this information, that's probably OK. Agreed. 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] Recovery inconsistencies, standby much larger than primary
On 2014-02-14 22:30:45 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-02-14 20:46:01 +, Greg Stark wrote: Going over this I think this is still a potential issue: On 31 Jan 2014 15:56, Andres Freund and...@2ndquadrant.com wrote: I am not sure that explains the issue, but I think the redo action for truncation is not safe across crashes. A XLOG_SMGR_TRUNCATE will just do a smgrtruncate() (and then mdtruncate) which will iterate over the segments starting at 0 till mdnblocks()/segment_size and *truncate* but not delete individual segment files that are not needed anymore, right? If we crash in the midst of that a new mdtruncate() will be issued, but it will get a shorter value back from mdnblocks(). We could probably fix things so it deleted backwards; it'd be a tad tedious because the list structure isn't organized that way, but we could do it. We could just make the list a doubly linked one, that'd make it simple. Not sure if that's good enough though. If you don't want to assume the filesystem metadata is coherent after a crash, we might have nonzero-size segments after zero-size ones, even if the truncate calls had been issued in the right order. I don't think that can actually happen on any realistic/interesting FS. Metadata updates better be journaled, so while they might not persist because the journal wasn't flushed, they should be applied in a sane order after a crash. But nonetheless I am not sure we want to rely on that. Another possibility is to keep opening and truncating files until we don't find the next segment in sequence, looking directly at the filesystem not at the mdfd chain. I don't think this would be appropriate in normal operation, but we could do it if InRecovery (and maybe even only if we don't think the database is consistent?) Yes, I was thinking of simply having a mdnblocks() variant that looks for the last existing file, disregarding the size. But looking around, it seems mdunlinkfork() has a similar issue, and I don't see how such a trick could be applied there :( I guess the theoretically correct thing would be to make all WAL records about truncation and unlinking contain the current size of the relation, but especially with deletions and forks that will probably turn out to be annoying to do. 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] narwhal and PGDLLIMPORT
On 2014-02-12 14:11:10 -0500, Tom Lane wrote: Marco Atzeri marco.atz...@gmail.com writes: On 12/02/2014 19:19, Andres Freund wrote: On 2014-02-12 19:13:07 +0100, Marco Atzeri wrote: About PGDLLIMPORT , my build log is full of warning: ‘optarg’ redeclared without dllimport attribute: previous dllimport ignored That should be fixed then. I guess cygwin's getopt.h declares it that way? from /usr/include/getopt.h extern char __declspec(dllimport) *optarg; /* argument associated with option */ Hm. All of our files that use getopt also do extern char *optarg; extern intoptind, opterr; #ifdef HAVE_INT_OPTRESET extern intoptreset;/* might not be declared by system headers */ #endif At least zic.c doesn't... So, now that brolga is revived, this unsurprisingly causes breakage there after the --disable-auto-export change. ISTM the easiest way to deal with this is to just adorn all those with a PGDLLIMPORT? I don't immediately see how the HAVE_INT_OPTRESET stuff could be reused in any interesting way as it's really independent of optind/optarg whether it exists? It strikes me that we should just move all that to src/include/getopt_long.h and include that from the places that currently include getopt.h directly. It's pretty pointless to have all those externs, HAVE_GETOPT_H, HAVE_INT_OPTRESET in every file. It'd be prettier if it were named getopt.h, but well... 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] commit fest 2014-01 week 4 report
Last week: Status Summary. Needs Review: 47, Waiting on Author: 15, Ready for Committer: 12, Committed: 37, Returned with Feedback: 3, Rejected: 2. Total: 116. This week: Status Summary. Needs Review: 45, Waiting on Author: 13, Ready for Committer: 14, Committed: 38, Returned with Feedback: 4, Rejected: 2. Total: 116. So this is going nowhere. We are now at the (possible) half-way point. So I'm going to start going through the patches and start forcing some decisions. We should start developing a desired timeline for our release. -- 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] New option for pg_basebackup, to specify a different directory for pg_xlog
On 2/9/14, 11:16 PM, Haribabu Kommi wrote: I also felt a lot of work for little benefit but as of now I am not able to find an easier solution to handle this problem. can we handle the same later if it really requires? I think we leave everything as is. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Display sequence belonging to table
Hi all: I just retrieved TODO list and find one item that refer to 'Display sequence owner'. Is there anyone working on it? I think I am interested in this. [ http://www.postgresql.org/message-id/1228622212.10877.59.ca...@godzilla.local.scalefeather.com] -- Rugal Bernstein Java Developer; Mysql/Oracle DBA; C/Java/Python/Bash; Vim; Linux; Pianist; Hadoop/Spark/Storm blog http://rugal.ml github https://github.com/Rugal twitterhttps://twitter.com/ryujinwrath gmail ryujinwr...@gmail.com
Re: [HACKERS] [9.3 bug] disk space in pg_xlog increases during archive recovery
On 2014-02-12 13:33:31 +0100, Andres Freund wrote: On 2014-02-12 21:23:54 +0900, MauMau wrote: Maybe we could consider in that direction, but there is a problem. Archive recovery slows down compared to 9.1, because of repeated restartpoints. Archive recovery should be as fast as possible, because it typically applies dozens or hundreds of WAL files, and the DBA desires immediate resumption of operation. So, I think we should restore 9.1 behavior for archive recovery. It's easy to be fast, if it's not correct. I don't see how that behaviour is acceptable, imo the previous behaviour simply was a bug whose fix was too invasive to backpatch. I don't think you can convince me (but maybe others) that the old behaviour is acceptable. I'm going to mark this patch as Rejected. Please speak up if that's not accurate. 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] Create function prototype as part of PG_FUNCTION_INFO_V1
Hi, On 2014-01-15 00:41:41 -0500, Tom Lane wrote: Peter Eisentraut pete...@gmx.net writes: This idea has appeared at least twice now, in http://www.postgresql.org/message-id/1386301050.2743.17.ca...@vanquo.pezone.net and http://www.postgresql.org/message-id/52d25aa2.50...@2ndquadrant.com . Even if it doesn't help with Windows issues, as discussed in the second thread, it still seems like a win for reducing boilerplate and accidental compiler warnings. So here is a patch for consideration. Meh. I don't think that extension authors are really going to appreciate changing from thou shalt declare all thy functions to thou shalt declare none of them. If the code were such that it wouldn't matter whether a manual declaration were provided too, then that wouldn't be a big deal --- but you seem to be ignoring the discussion in the one thread cited above about PGDLLEXPORT. Also, surely it is 100% bogus for fmgr.h to be declaring functions not actually provided by fmgr.c. That will create about as many failure modes as it removes, not to mention being conceptually wrong. The latter point might possibly be worked around by putting the externs for _PG_init and _PG_fini into the PG_MODULE_MAGIC macro, though I'm not sure how well that works for multi-source-file extensions; the init functions might be in some other file than the PG_MODULE_MAGIC call. Based on those comments and the lack of counter arguments after a month I am going to mark the patch as rejected. 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] Exposing currentTransactionWALVolume
Hi Simon,
On 2014-01-14 17:12:35 +, Simon Riggs wrote:
/*
- * MarkCurrentTransactionIdLoggedIfAny
+ * ReportTransactionInsertedWAL
*
- * Remember that the current xid - if it is assigned - now has been wal
logged.
+ * Remember that the current xid - if it is assigned - has now inserted WAL
*/
void
-MarkCurrentTransactionIdLoggedIfAny(void)
+ReportTransactionInsertedWAL(uint32 insertedWALVolume)
{
+ currentTransactionWALVolume += insertedWALVolume;
if (TransactionIdIsValid(CurrentTransactionState-transactionId))
CurrentTransactionState-didLogXid = true;
}
Not a big fan of combining those two. One works on the toplevel
transaction, the other on the current subtransaction... The new name
also ignores that it's only taking effect if there's actually a
transaction in progress.
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] New hook after raw parsing, before analyze
2014.02.15. dátummal, 0:46 időpontban Greg Stark st...@mit.edu írta: On Fri, Feb 14, 2014 at 9:16 PM, David Beck db...@starschema.net wrote: Another point I liked in mysql is the possibility to write info schema plugins: http://dev.mysql.com/doc/refman/5.1/en/writing-information-schema-plugins.html Like a virtual catalog. Is there anything similar in Postgres? The documentation you linked to describes how to provide information_schema plugins but not why you would want to do such a thing. I'm not seeing why this would be useful. The information_schema schema is described by the standard so creating new views in it isn't needed very often and the schema for the existing views doesn't change very often. I can see why a plugin might want to add rows to the views but that doesn't seem to be what this feature is about. Another reason I was thinking about dynamic catalog and/or query rewrite is the project I work on is a data integration platform. Right now it is in the feasibility study phase and Postgres+extension looks to be the strongest option. The legacy system we want to interface with has over 150k table like objects. Our platform’s task is to provide a relational view on top of them. I know that it is unlikely the users to use all 150k tables. I would expect may be 10-100 are used in practice, but I didn’t want to figure out which 100, neither want to create all 150k catalog entries in advance. I was also dreaming about the possibility to transfer the small enough objects to Postgres tables in the background and spare the communication with the legacy system and let Postgres do the joins on these. The solution I was thinking about is this: - when the query arrives a smart rewrite would know 1) what tables are local 2) what tables need new catalog entries 3) what can be joined on the other side - the rewriter would potentially add SQL statements in the beginning of the query for creating the missing FDW catalog entries if needed - the FDW would be handled by the same extension so they can easily talk to each other about the status of the objects, so the rewriter would know if the background transfer of the small table is completed and should do the rewrite accordingly I know these are pretty far from the functionality and traditional operation of an RDBMS… but if you look at the FDW examples like do a select on a Google Imap mailbox, it is not that far from Postgres Best regards, David -- 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] CREATE FOREIGN TABLE ( ... LIKE ... )
On 2014-01-31 18:16:18 +0100, Vik Fearing wrote: On 01/25/2014 06:25 AM, David Fetter wrote: Please find attached the next rev :) This version looks committable to me, so I am marking it as such. This doesn't contain a single regression test, I don't see how that's ok. Marking as waiting on author. 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] commit fest 2014-01 week 4 report
Hi, On 2014-02-15 08:34:19 -0500, Peter Eisentraut wrote: Status Summary. Needs Review: 47, Waiting on Author: 15, Ready for Committer: 12, Committed: 37, Returned with Feedback: 3, Rejected: 2. Total: 116. This week: Status Summary. Needs Review: 45, Waiting on Author: 13, Ready for Committer: 14, Committed: 38, Returned with Feedback: 4, Rejected: 2. Total: 116. So this is going nowhere. We are now at the (possible) half-way point. So I'm going to start going through the patches and start forcing some decisions. I'd just started making a pass before your email, sorry. I think we really need to start punting Waiting for Author patches and nontrivial patches !bugfix patches that are new in this CF. I think we should also unassign reviews that didn't do anything so far... 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] Performance Improvement by reducing WAL for Update Operation
Hi,
Some quick review comments:
On 2014-02-13 18:14:54 +0530, Amit Kapila wrote:
+ /*
+ * EWT can be generated for all new tuple versions created by Update
+ * operation. Currently we do it when both the old and new tuple
versions
+ * are on same page, because during recovery if the page containing old
+ * tuple is corrupt, it should not cascade that corruption to other
pages.
+ * Under the general assumption that for long runs most updates tend to
+ * create new tuple version on same page, there should not be
significant
+ * impact on WAL reduction or performance.
+ *
+ * We should not generate EWT when we need to backup the whole block in
+ * WAL as in that case there is no saving by reduced WAL size.
+ */
+
+ if (RelationIsEnabledForWalCompression(reln)
+ (oldbuf == newbuf)
+ !XLogCheckBufferNeedsBackup(newbuf))
+ {
+ uint32 enclen;
You should note that thew check for RelationIsEnabledForWalCompression()
here is racy and that that's ok because the worst that can happen is
that a uselessly generated delta.
xlrec.target.node = reln-rd_node;
xlrec.target.tid = oldtup-t_self;
xlrec.old_xmax = HeapTupleHeaderGetRawXmax(oldtup-t_data);
@@ -6619,6 +6657,8 @@ log_heap_update(Relation reln, Buffer oldbuf,
xlrec.newtid = newtup-t_self;
if (new_all_visible_cleared)
xlrec.flags |= XLOG_HEAP_NEW_ALL_VISIBLE_CLEARED;
+ if (compressed)
+ xlrec.flags |= XLOG_HEAP_DELTA_ENCODED;
I think this also needs to unset XLOG_HEAP_CONTAINS_NEW_TUPLE and
conditional on !need_tuple_data.
/*
+ * Determine whether the buffer referenced has to be backed up. Since we
don't
+ * yet have the insert lock, fullPageWrites and forcePageWrites could change
+ * later, but will not cause any problem because this function is used only
to
+ * identify whether EWT is required for update.
+ */
+bool
+XLogCheckBufferNeedsBackup(Buffer buffer)
+{
Should note very, very boldly that this can only be used in contexts
where a race is acceptable.
diff --git a/src/backend/utils/adt/pg_rbcompress.c
b/src/backend/utils/adt/pg_rbcompress.c
new file mode 100644
index 000..877ccd7
--- /dev/null
+++ b/src/backend/utils/adt/pg_rbcompress.c
@@ -0,0 +1,355 @@
+/* --
+ * pg_rbcompress.c -
+ *
+ * This is a delta encoding scheme specific to PostgreSQL and
designed
+ * to compress similar tuples. It can be used as it is or extended
for
+ * other purpose in PostgrSQL if required.
+ *
+ * Currently, this just checks for a common prefix and/or suffix,
but
+ * the output format is similar to the LZ format used in
pg_lzcompress.c.
+ *
+ * Copyright (c) 1999-2014, PostgreSQL Global Development Group
+ *
+ * src/backend/utils/adt/pg_rbcompress.c
+ * --
+ */
This needs significantly more explanations about the algorithm and the
reasoning behind it.
+static const PGRB_Strategy strategy_default_data = {
+ 32, /* Data chunks
less than 32 bytes are not
+ * compressed */
+ INT_MAX,/* No upper limit on
what we'll try to
+ * compress */
+ 35, /* Require 25%
compression rate, or not worth
+ * it */
+};
compression rate looks like it's mismatch between comment and code.
+/* --
+ * pgrb_out_ctrl -
+ *
+ * Outputs the last and allocates a new control byte if needed.
+ * --
+ */
+#define pgrb_out_ctrl(__ctrlp,__ctrlb,__ctrl,__buf) \
+do { \
+ if ((__ctrl 0xff) == 0)
\
+ {
\
+ *(__ctrlp) = __ctrlb;
\
+ __ctrlp = (__buf)++;
\
+ __ctrlb = 0;
\
+ __ctrl = 1;
\
+ }
\
+} while (0)
+
double underscore variables are reserved for the
Re: [HACKERS] Create function prototype as part of PG_FUNCTION_INFO_V1
On 2/15/14, 8:51 AM, Andres Freund wrote: Based on those comments and the lack of counter arguments after a month I am going to mark the patch as rejected. Actually, I was waiting for that PGDLLIMPORT thread to sort itself out before tackling this. -- 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
Andres Freund and...@2ndquadrant.com writes: My current conclusion is that backporting barriers.h is by far the most reasonable way to go. The compiler problems have been ironed out by now... -1. IMO that code is still quite unproven, and what's more, the problem we're discussing here is completely hypothetical. If it were real, we'd have field evidence of it. We've not had that much trouble seeing instances of even very narrow race-condition windows in the past. 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] Create function prototype as part of PG_FUNCTION_INFO_V1
On 1/15/14, 12:41 AM, Tom Lane wrote: Meh. I don't think that extension authors are really going to appreciate changing from thou shalt declare all thy functions to thou shalt declare none of them. This patch does no such thing. If the code were such that it wouldn't matter whether a manual declaration were provided too, then that wouldn't be a big deal --- but you seem to be ignoring the discussion in the one thread cited above about PGDLLEXPORT. In that thread, you argue that requiring PGDLLEXPORT is not acceptable, so that makes this argument irrelevant. Also, surely it is 100% bogus for fmgr.h to be declaring functions not actually provided by fmgr.c. That's the arrangement if you don't have dynamic loading. For dynamic loading, the question isn't necessarily who provides them, but who determines the interface for them. There has to be a way for the postgres backend to say to extension module authors, I'm going to try to load this function, and this is what it should look like. This case wasn't envisioned when they designed C in 1970. If you have a better idea, I'm listening. That will create about as many failure modes as it removes, not to mention being conceptually wrong. What failure modes? -- 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] 9.2.1 index-only scans : abnormal heap fetches after VACUUM FULL
Hi,
*** end_heap_rewrite(RewriteState state)
*** 281,286
--- 284,290
true);
RelationOpenSmgr(state-rs_new_rel);
+ update_page_vm(state-rs_new_rel, state-rs_buffer,
state-rs_blockno);
PageSetChecksumInplace(state-rs_buffer, state-rs_blockno);
smgrextend(state-rs_new_rel-rd_smgr, MAIN_FORKNUM,
state-rs_blockno,
*** raw_heap_insert(RewriteState state, Heap
*** 633,638
--- 637,643
*/
RelationOpenSmgr(state-rs_new_rel);
+ update_page_vm(state-rs_new_rel, page,
state-rs_blockno);
PageSetChecksumInplace(page, state-rs_blockno);
smgrextend(state-rs_new_rel-rd_smgr, MAIN_FORKNUM,
I think those two cases should be combined.
+ static void
+ update_page_vm(Relation relation, Page page, BlockNumber blkno)
+ {
+ Buffer vmbuffer = InvalidBuffer;
+ TransactionId visibility_cutoff_xid;
+
+ visibilitymap_pin(relation, blkno, vmbuffer);
+ Assert(BufferIsValid(vmbuffer));
+
+ if (!visibilitymap_test(relation, blkno, vmbuffer)
+ heap_page_is_all_visible(relation, InvalidBuffer, page,
+
visibility_cutoff_xid))
+ {
+ PageSetAllVisible(page);
+ visibilitymap_set(relation, blkno, InvalidBuffer,
+ InvalidXLogRecPtr, vmbuffer,
visibility_cutoff_xid);
+ }
+ ReleaseBuffer(vmbuffer);
+ }
How could visibilitymap_test() be true here?
diff --git a/src/backend/access/heap/visibilitymap.c
b/src/backend/access/heap/visibilitymap.c
new file mode 100644
index 899ffac..3e7546b
*** a/src/backend/access/heap/visibilitymap.c
--- b/src/backend/access/heap/visibilitymap.c
*** visibilitymap_set(Relation rel, BlockNum
*** 257,263
#endif
Assert(InRecovery || XLogRecPtrIsInvalid(recptr));
- Assert(InRecovery || BufferIsValid(heapBuf));
/* Check that we have the right heap page pinned, if present */
if (BufferIsValid(heapBuf) BufferGetBlockNumber(heapBuf) != heapBlk)
--- 257,262
*** visibilitymap_set(Relation rel, BlockNum
*** 278,284
map[mapByte] |= (1 mapBit);
MarkBufferDirty(vmBuf);
! if (RelationNeedsWAL(rel))
{
if (XLogRecPtrIsInvalid(recptr))
{
--- 277,283
map[mapByte] |= (1 mapBit);
MarkBufferDirty(vmBuf);
! if (RelationNeedsWAL(rel) BufferIsValid(heapBuf))
{
if (XLogRecPtrIsInvalid(recptr))
{
At the very least this needs to revise visibilitymap_set's comment about
the requirement of passing a buffer unless !InRecovery.
Also, how is this going to work with replication if you're explicitly
not WAL logging this?
*** vac_cmp_itemptr(const void *left, const
*** 1730,1743
* transactions. Also return the visibility_cutoff_xid which is the highest
* xmin amongst the visible tuples.
*/
! static bool
! heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId
*visibility_cutoff_xid)
{
- Pagepage = BufferGetPage(buf);
OffsetNumber offnum,
maxoff;
boolall_visible = true;
*visibility_cutoff_xid = InvalidTransactionId;
/*
--- 1728,1744
* transactions. Also return the visibility_cutoff_xid which is the highest
* xmin amongst the visible tuples.
*/
! bool
! heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
! TransactionId
*visibility_cutoff_xid)
{
OffsetNumber offnum,
maxoff;
boolall_visible = true;
+ if (BufferIsValid(buf))
+ page = BufferGetPage(buf);
+
*visibility_cutoff_xid = InvalidTransactionId;
/*
*** heap_page_is_all_visible(Relation rel, B
*** 1758,1764
if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
continue;
! ItemPointerSet((tuple.t_self), BufferGetBlockNumber(buf),
offnum);
/*
* Dead line pointers can have index pointers pointing to them.
So
--- 1759,1767
if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
continue;
! /* XXX use 0 or real offset? */
! ItemPointerSet((tuple.t_self), BufferIsValid(buf) ?
!BufferGetBlockNumber(buf) : 0,
offnum);
How about passing in the page and block
Re: [HACKERS] narwhal and PGDLLIMPORT
Andres Freund and...@2ndquadrant.com writes: On 2014-02-12 14:11:10 -0500, Tom Lane wrote: Marco Atzeri marco.atz...@gmail.com writes: from /usr/include/getopt.h extern char __declspec(dllimport) *optarg; /* argument associated with option */ Hm. All of our files that use getopt also do extern char *optarg; So, now that brolga is revived, this unsurprisingly causes breakage there after the --disable-auto-export change. ISTM the easiest way to deal with this is to just adorn all those with a PGDLLIMPORT? Uh, no, because that's backwards: we'd need the __declspec(dllimport) in the backend code. The best thing probably is not to have the duplicate declarations on platforms that don't need 'em. Unfortunately, I seem to recall that the current coding was arrived at to forestall link problems on weird platforms that *had* these symbols declared and yet we needed externs anyway. We might have to do something as ugly as #ifndef CYGWIN. 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] Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease
On 2014-02-15 10:06:41 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: My current conclusion is that backporting barriers.h is by far the most reasonable way to go. The compiler problems have been ironed out by now... -1. IMO that code is still quite unproven, and what's more, the problem we're discussing here is completely hypothetical. If it were real, we'd have field evidence of it. We've not had that much trouble seeing instances of even very narrow race-condition windows in the past. Well, the problem is that few of us have access to interesting !x86 machines to run tests, and that's where we'd see problems (since x86 gives enough guarantees to avoid this unless the compiler reorders stuff). I am personally fine with just using volatiles to avoid reordering in the older branches, but Florian argued against it. 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] Create function prototype as part of PG_FUNCTION_INFO_V1
Peter Eisentraut pete...@gmx.net writes: On 1/15/14, 12:41 AM, Tom Lane wrote: Meh. I don't think that extension authors are really going to appreciate changing from thou shalt declare all thy functions to thou shalt declare none of them. This patch does no such thing. Yes it does; people who fail to remove their manual externs will get Windows-only build failures (or at least warnings; it's not very clear which declaration will win). 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 2014-02-15 10:16:50 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-02-12 14:11:10 -0500, Tom Lane wrote: Marco Atzeri marco.atz...@gmail.com writes: from /usr/include/getopt.h extern char __declspec(dllimport) *optarg; /* argument associated with option */ Hm. All of our files that use getopt also do extern char *optarg; So, now that brolga is revived, this unsurprisingly causes breakage there after the --disable-auto-export change. ISTM the easiest way to deal with this is to just adorn all those with a PGDLLIMPORT? Uh, no, because that's backwards: we'd need the __declspec(dllimport) in the backend code. Yuck, it's even uglier than that. On normal windows it's not actually the platform's getopt() that ends up getting really used, but the one from port/... # mingw has adopted a GNU-centric interpretation of optind/optreset, # so always use our version on Windows. if test $PORTNAME = win32; then AC_LIBOBJ(getopt) AC_LIBOBJ(getopt_long) fi The best thing probably is not to have the duplicate declarations on platforms that don't need 'em. Unfortunately, I seem to recall that the current coding was arrived at to forestall link problems on weird platforms that *had* these symbols declared and yet we needed externs anyway. We might have to do something as ugly as #ifndef CYGWIN. Hm, according to a quick blame, they are there unconditionally since at least 2000 (c.f. a70e74b06 moving them around). So it very well might be that that reasoning isn't current anymore. One ugly thing to do is to fall back to the port implementation of getopt on cygwin as well... That'd still have the warning parade tho. 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] Performance Improvement by reducing WAL for Update Operation
On Sat, Feb 15, 2014 at 8:21 PM, Andres Freund and...@2ndquadrant.com wrote: Hi, Some quick review comments: Thanks for the review, I shall handle/reply to comments with the updated version in which I am planing to fix a bug (right now preparing a test to reproduce it) in this code. Bug: Tag can handle maximum length of 273 bytes, but this patch is not considering it. I have to admit, I have serious doubts about this approach. I have a very hard time believing this won't cause performance regression in many common cases... Actually, till now I was majorly focusing on worst case (i.e at boundary of compression ratio) thinking that most others will do good. However I shall produce data for much more common cases as well. Please let me know if you have anything specific thing in mind where this will not work well. More importantly I don't think doing the compression on this level is that interesting. I know Heikki argued for it, but I think extending the bitmap that's computed for HOT to cover all columns and doing this on a column level sounds much more sensible to me. Previously we have tried to do at column boundaries, but the main problem turned out to be in worst cases where we spend time in extracting values from tuples based on column boundaries and later found that data is not compressible. 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] Performance Improvement by reducing WAL for Update Operation
On 2014-02-15 21:01:07 +0530, Amit Kapila wrote: More importantly I don't think doing the compression on this level is that interesting. I know Heikki argued for it, but I think extending the bitmap that's computed for HOT to cover all columns and doing this on a column level sounds much more sensible to me. Previously we have tried to do at column boundaries, but the main problem turned out to be in worst cases where we spend time in extracting values from tuples based on column boundaries and later found that data is not compressible. I think that hugely depends on how you implement it. I think you'd need to have a loop traversing over the both tuples at the same time on the level of heap_deform_tuple(). If you'd use the result to get rid of HeapSatisfiesHOTandKeyUpdate() at the same time I am pretty sure you wouldn't see very high overhead. 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] Create function prototype as part of PG_FUNCTION_INFO_V1
On 2/15/14, 10:22 AM, Tom Lane wrote: Peter Eisentraut pete...@gmx.net writes: On 1/15/14, 12:41 AM, Tom Lane wrote: Meh. I don't think that extension authors are really going to appreciate changing from thou shalt declare all thy functions to thou shalt declare none of them. This patch does no such thing. Yes it does; people who fail to remove their manual externs will get Windows-only build failures (or at least warnings; it's not very clear which declaration will win). The manual externs and the automatically provided ones are exactly the same. Why would that fail? -- 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
Andres Freund and...@2ndquadrant.com writes: On 2014-02-15 10:16:50 -0500, Tom Lane wrote: The best thing probably is not to have the duplicate declarations on platforms that don't need 'em. Unfortunately, I seem to recall that the current coding was arrived at to forestall link problems on weird platforms that *had* these symbols declared and yet we needed externs anyway. We might have to do something as ugly as #ifndef CYGWIN. Hm, according to a quick blame, they are there unconditionally since at least 2000 (c.f. a70e74b06 moving them around). So it very well might be that that reasoning isn't current anymore. I don't have time right now to research it (have to go shovel snow), but I think that at least some of the issue was that we needed the externs when we force use of our src/port implementation. One ugly thing to do is to fall back to the port implementation of getopt on cygwin as well... That'd still have the warning parade tho. Yeah, that doesn't sound terribly satisfactory. Another idea would be to wrap the externs in #ifndef HAVE_GETOPT_H. 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 2014-02-15 10:59:17 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-02-15 10:16:50 -0500, Tom Lane wrote: The best thing probably is not to have the duplicate declarations on platforms that don't need 'em. Unfortunately, I seem to recall that the current coding was arrived at to forestall link problems on weird platforms that *had* these symbols declared and yet we needed externs anyway. We might have to do something as ugly as #ifndef CYGWIN. Hm, according to a quick blame, they are there unconditionally since at least 2000 (c.f. a70e74b06 moving them around). So it very well might be that that reasoning isn't current anymore. I don't have time right now to research it (have to go shovel snow), but I think that at least some of the issue was that we needed the externs when we force use of our src/port implementation. I think that'd be solvable easy enough if we'd just always included pg's getopt_long.h (or a new getopt.h) which properly deals with defining them when included. That'd centralize all the magic and it'd overall get rid of a ton of ifdefs and externs. 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] Problem with displaying wide tables in psql
Hi,
This is my review about 3th version of the patch. It is an useful
improvement in my opinion. It worked well on my environment.
2013-12-11 17:43:06, Sergey Muraviov sergey.k.murav...@gmail.com:
It works in expanded mode when either format option is set to wrapped
(\pset format wrapped), or we have no pager, or pager doesn't chop long
lines (so you can still use the trick).
I do not like this logic on the IsWrappingNeeded function. It does not
seems right to check the environment variables for less. It would be hard
to explain this behavior to the users. It is better to make this only
the behavior of the wrapped format in expanded mode, in my opinion.
{
if (opt_border 2)
fprintf(fout, %s\n,
dlineptr[line_count].ptr);
else
fprintf(fout, %-s%*s %s\n,
dlineptr[line_count].ptr,
dwidth -
dlineptr[line_count].width, ,
dformat-rightvrule);
}
Is it necessary to keep this old print line code? It seems to me the new
code works well on (dlineptr[line_count].width = dwidth) condition.
--
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-15 16:18:00 +0100, Andres Freund wrote:
On 2014-02-15 10:06:41 -0500, Tom Lane wrote:
Andres Freund and...@2ndquadrant.com writes:
My current conclusion is that backporting barriers.h is by far the most
reasonable way to go. The compiler problems have been ironed out by
now...
-1. IMO that code is still quite unproven, and what's more, the
problem we're discussing here is completely hypothetical. If it
were real, we'd have field evidence of it. We've not had that
much trouble seeing instances of even very narrow race-condition
windows in the past.
Well, the problem is that few of us have access to interesting !x86
machines to run tests, and that's where we'd see problems (since x86
gives enough guarantees to avoid this unless the compiler reorders
stuff). I am personally fine with just using volatiles to avoid
reordering in the older branches, but Florian argued against it.
Here's patches doing that. The 9.3 version also applies to 9.2; the 9.1
version applies back to 8.4.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 0fe7ce4..a8d5b7f 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -647,12 +647,27 @@ LWLockRelease(LWLockId lockid)
*/
while (head != NULL)
{
+ /*
+ * Use volatile to prevent the compiler from reordering the store to
+ * lwWaitLink with the store to lwWaiting which could cause problems
+ * when the to-be-woken-up backend wakes up spuriously and writes to
+ * lwWaitLink when acquiring a new lock. That could corrupt the list
+ * this backend is traversing leading to backends stuck waiting for a
+ * lock.
+ *
+ * That's not neccessarily sufficient for out-of-order architectures,
+ * but there've been no field report of problems. The proper solution
+ * would be to use a write barrier, but those are not available in the
+ * back branches.
+ */
+ volatile PGPROC *vp = proc;
+
LOG_LWDEBUG(LWLockRelease, lockid, release waiter);
- proc = head;
- head = proc-lwWaitLink;
- proc-lwWaitLink = NULL;
- proc-lwWaiting = false;
- PGSemaphoreUnlock(proc-sem);
+ vp = head;
+ head = vp-lwWaitLink;
+ vp-lwWaitLink = NULL;
+ vp-lwWaiting = false;
+ PGSemaphoreUnlock(vp-sem);
}
/*
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 4f88d3f..cff631d 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -27,6 +27,7 @@
#include commands/async.h
#include miscadmin.h
#include pg_trace.h
+#include storage/barrier.h
#include storage/ipc.h
#include storage/predicate.h
#include storage/proc.h
@@ -831,10 +832,21 @@ LWLockRelease(LWLockId lockid)
*/
while (head != NULL)
{
+ /*
+ * Use a write barrier to prevent the compiler from reordering the
+ * store to lwWaitLink with the store to lwWaiting which could cause
+ * problems when the to-be-woken-up backend wakes up spuriously and
+ * writes to lwWaitLink when acquiring a new lock. That could corrupt
+ * the list this backend is traversing leading to backends stuck
+ * waiting for a lock. A write barrier is sufficient as the read side
+ * only accesses the data while holding a spinlock which acts as a
+ * full barrier.
+ */
LOG_LWDEBUG(LWLockRelease, lockid, release waiter);
proc = head;
head = proc-lwWaitLink;
proc-lwWaitLink = NULL;
+ pg_write_barrier();
proc-lwWaiting = false;
PGSemaphoreUnlock(proc-sem);
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 85a0ce9..22f8540 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1872,9 +1872,11 @@ WakeupWaiters(XLogRecPtr EndPos)
*/
while (head != NULL)
{
+ /* check comment in LWLockRelease() about barrier usage */
proc = head;
head = proc-lwWaitLink;
proc-lwWaitLink = NULL;
+ pg_write_barrier();
proc-lwWaiting = false;
PGSemaphoreUnlock(proc-sem);
}
@@ -1966,9 +1968,11 @@ WALInsertSlotReleaseOne(int slotno)
*/
while (head != NULL)
{
+ /* check comment in LWLockRelease() about barrier usage */
proc = head;
head = proc-lwWaitLink;
proc-lwWaitLink = NULL;
+ pg_write_barrier();
proc-lwWaiting = false;
PGSemaphoreUnlock(proc-sem);
}
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 82ef440..98c4845 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -28,6 +28,7 @@
#include miscadmin.h
#include pg_trace.h
#include replication/slot.h
+#include storage/barrier.h
#include storage/ipc.h
#include storage/predicate.h
#include storage/proc.h
@@ -944,10 +945,21 @@ LWLockRelease(LWLock *l)
*/
while (head != NULL)
{
+ /*
+ * Use a write barrier to prevent the compiler
Re: [HACKERS] narwhal and PGDLLIMPORT
Andres Freund and...@2ndquadrant.com writes: On 2014-02-15 10:59:17 -0500, Tom Lane wrote: I don't have time right now to research it (have to go shovel snow), but I think that at least some of the issue was that we needed the externs when we force use of our src/port implementation. I think that'd be solvable easy enough if we'd just always included pg's getopt_long.h (or a new getopt.h) which properly deals with defining them when included. That'd centralize all the magic and it'd overall get rid of a ton of ifdefs and externs. Yeah, there are enough copies of that stuff that centralizing them sounds like a great idea. Call it pg_getopt.h, perhaps? AFAICS, getopt_long.h just defines them unconditionally, which is as wrong as everyplace else. The reasonable alternatives seem to be (1) invent pg_getopt.h, which would #include getopt.h if available and then provide properly-ifdef'd externs for optarg and friends; getopt_long.h would #include pg_getopt.h. (2) Just do the above in getopt_long.h, and include it in the places that currently have externs for optarg and friends. Option (2) seems a bit odd in files that aren't actually using getopt_long(), but it avoids making a new header file. Preferences? 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] Create function prototype as part of PG_FUNCTION_INFO_V1
Peter Eisentraut pete...@gmx.net writes: On 2/15/14, 10:22 AM, Tom Lane wrote: Yes it does; people who fail to remove their manual externs will get Windows-only build failures (or at least warnings; it's not very clear which declaration will win). The manual externs and the automatically provided ones are exactly the same. Why would that fail? Maybe I'm remembering the wrong patch. I thought what this patch was intending was to put PGDLLEXPORT into the automatically-provided externs. 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 2014-02-15 12:16:58 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-02-15 10:59:17 -0500, Tom Lane wrote: I don't have time right now to research it (have to go shovel snow), but I think that at least some of the issue was that we needed the externs when we force use of our src/port implementation. I think that'd be solvable easy enough if we'd just always included pg's getopt_long.h (or a new getopt.h) which properly deals with defining them when included. That'd centralize all the magic and it'd overall get rid of a ton of ifdefs and externs. Yeah, there are enough copies of that stuff that centralizing them sounds like a great idea. Call it pg_getopt.h, perhaps? I'm just working on it. pg_getopt.h was exactly what I came up with. (1) invent pg_getopt.h, which would #include getopt.h if available and then provide properly-ifdef'd externs for optarg and friends; getopt_long.h would #include pg_getopt.h. That's what I've done. I'll post in a minute, just wanted to give a headsup so we don't duplicate work. 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] 9.2.1 index-only scans : abnormal heap fetches after VACUUM FULL
On Sat, Feb 15, 2014 at 04:16:40PM +0100, Andres Freund wrote:
Hi,
*** end_heap_rewrite(RewriteState state)
*** 281,286
--- 284,290
true);
RelationOpenSmgr(state-rs_new_rel);
+ update_page_vm(state-rs_new_rel, state-rs_buffer,
state-rs_blockno);
PageSetChecksumInplace(state-rs_buffer, state-rs_blockno);
smgrextend(state-rs_new_rel-rd_smgr, MAIN_FORKNUM,
state-rs_blockno,
*** raw_heap_insert(RewriteState state, Heap
*** 633,638
--- 637,643
*/
RelationOpenSmgr(state-rs_new_rel);
+ update_page_vm(state-rs_new_rel, page,
state-rs_blockno);
PageSetChecksumInplace(page, state-rs_blockno);
smgrextend(state-rs_new_rel-rd_smgr, MAIN_FORKNUM,
I think those two cases should be combined.
Uh, what I did was to pair the new update_page_vm with the existing
PageSetChecksumInplace calls, figuring if we needed a checksum before we
wrote the page we would need a update_page_vm too. Is that incorrect?
It is that _last_ page write in the second instance.
+ static void
+ update_page_vm(Relation relation, Page page, BlockNumber blkno)
+ {
+ Buffer vmbuffer = InvalidBuffer;
+ TransactionId visibility_cutoff_xid;
+
+ visibilitymap_pin(relation, blkno, vmbuffer);
+ Assert(BufferIsValid(vmbuffer));
+
+ if (!visibilitymap_test(relation, blkno, vmbuffer)
+ heap_page_is_all_visible(relation, InvalidBuffer, page,
+
visibility_cutoff_xid))
+ {
+ PageSetAllVisible(page);
+ visibilitymap_set(relation, blkno, InvalidBuffer,
+ InvalidXLogRecPtr, vmbuffer,
visibility_cutoff_xid);
+ }
+ ReleaseBuffer(vmbuffer);
+ }
How could visibilitymap_test() be true here?
Oh, you are right that I can only hit that once per page; test removed.
diff --git a/src/backend/access/heap/visibilitymap.c
b/src/backend/access/heap/visibilitymap.c
new file mode 100644
index 899ffac..3e7546b
*** a/src/backend/access/heap/visibilitymap.c
--- b/src/backend/access/heap/visibilitymap.c
*** visibilitymap_set(Relation rel, BlockNum
*** 257,263
#endif
Assert(InRecovery || XLogRecPtrIsInvalid(recptr));
- Assert(InRecovery || BufferIsValid(heapBuf));
/* Check that we have the right heap page pinned, if present */
if (BufferIsValid(heapBuf) BufferGetBlockNumber(heapBuf) != heapBlk)
--- 257,262
*** visibilitymap_set(Relation rel, BlockNum
*** 278,284
map[mapByte] |= (1 mapBit);
MarkBufferDirty(vmBuf);
! if (RelationNeedsWAL(rel))
{
if (XLogRecPtrIsInvalid(recptr))
{
--- 277,283
map[mapByte] |= (1 mapBit);
MarkBufferDirty(vmBuf);
! if (RelationNeedsWAL(rel) BufferIsValid(heapBuf))
{
if (XLogRecPtrIsInvalid(recptr))
{
At the very least this needs to revise visibilitymap_set's comment about
the requirement of passing a buffer unless !InRecovery.
Oh, good point, comment block updated.
Also, how is this going to work with replication if you're explicitly
not WAL logging this?
Uh, I assumed that using the existing functions would handle this. If
not, I don't know the answer.
*** vac_cmp_itemptr(const void *left, const
*** 1730,1743
* transactions. Also return the visibility_cutoff_xid which is the
highest
* xmin amongst the visible tuples.
*/
! static bool
! heap_page_is_all_visible(Relation rel, Buffer buf, TransactionId
*visibility_cutoff_xid)
{
- Pagepage = BufferGetPage(buf);
OffsetNumber offnum,
maxoff;
boolall_visible = true;
*visibility_cutoff_xid = InvalidTransactionId;
/*
--- 1728,1744
* transactions. Also return the visibility_cutoff_xid which is the
highest
* xmin amongst the visible tuples.
*/
! bool
! heap_page_is_all_visible(Relation rel, Buffer buf, Page page,
!TransactionId
*visibility_cutoff_xid)
{
OffsetNumber offnum,
maxoff;
boolall_visible = true;
+ if (BufferIsValid(buf))
+ page = BufferGetPage(buf);
+
*visibility_cutoff_xid = InvalidTransactionId;
/*
*** heap_page_is_all_visible(Relation rel, B
*** 1758,1764
if (!ItemIdIsUsed(itemid) || ItemIdIsRedirected(itemid))
Re: [HACKERS] 9.2.1 index-only scans : abnormal heap fetches after VACUUM FULL
On 2014-02-15 12:50:14 -0500, Bruce Momjian wrote:
On Sat, Feb 15, 2014 at 04:16:40PM +0100, Andres Freund wrote:
Hi,
*** end_heap_rewrite(RewriteState state)
*** 281,286
--- 284,290
true);
RelationOpenSmgr(state-rs_new_rel);
+ update_page_vm(state-rs_new_rel, state-rs_buffer,
state-rs_blockno);
PageSetChecksumInplace(state-rs_buffer,
state-rs_blockno);
smgrextend(state-rs_new_rel-rd_smgr, MAIN_FORKNUM,
state-rs_blockno,
*** raw_heap_insert(RewriteState state, Heap
*** 633,638
--- 637,643
*/
RelationOpenSmgr(state-rs_new_rel);
+ update_page_vm(state-rs_new_rel, page,
state-rs_blockno);
PageSetChecksumInplace(page, state-rs_blockno);
smgrextend(state-rs_new_rel-rd_smgr,
MAIN_FORKNUM,
I think those two cases should be combined.
Uh, what I did was to pair the new update_page_vm with the existing
PageSetChecksumInplace calls, figuring if we needed a checksum before we
wrote the page we would need a update_page_vm too. Is that incorrect?
It is that _last_ page write in the second instance.
What I mean is that there should be a new function handling the writeout
of a page.
diff --git a/src/backend/access/heap/visibilitymap.c
b/src/backend/access/heap/visibilitymap.c
new file mode 100644
index 899ffac..3e7546b
*** a/src/backend/access/heap/visibilitymap.c
--- b/src/backend/access/heap/visibilitymap.c
*** visibilitymap_set(Relation rel, BlockNum
*** 257,263
#endif
Assert(InRecovery || XLogRecPtrIsInvalid(recptr));
- Assert(InRecovery || BufferIsValid(heapBuf));
/* Check that we have the right heap page pinned, if present */
if (BufferIsValid(heapBuf) BufferGetBlockNumber(heapBuf) !=
heapBlk)
--- 257,262
*** visibilitymap_set(Relation rel, BlockNum
*** 278,284
map[mapByte] |= (1 mapBit);
MarkBufferDirty(vmBuf);
! if (RelationNeedsWAL(rel))
{
if (XLogRecPtrIsInvalid(recptr))
{
--- 277,283
map[mapByte] |= (1 mapBit);
MarkBufferDirty(vmBuf);
! if (RelationNeedsWAL(rel) BufferIsValid(heapBuf))
{
if (XLogRecPtrIsInvalid(recptr))
{
At the very least this needs to revise visibilitymap_set's comment about
the requirement of passing a buffer unless !InRecovery.
Oh, good point, comment block updated.
Also, how is this going to work with replication if you're explicitly
not WAL logging this?
Uh, I assumed that using the existing functions would handle this. If
not, I don't know the answer.
Err. Read the block above again, where you added the
BufferIsValid(heapBuf) check. That's exactly the part doing the WAL
logging.
*** a/src/backend/utils/time/tqual.c
--- b/src/backend/utils/time/tqual.c
*** static inline void
*** 108,113
--- 108,117
SetHintBits(HeapTupleHeader tuple, Buffer buffer,
uint16 infomask, TransactionId xid)
{
+ /* we might not have a buffer if we are doing raw_heap_insert()
*/
+ if (!BufferIsValid(buffer))
+ return;
+
if (TransactionIdIsValid(xid))
{
/* NB: xid must be known committed here! */
This looks seriously wrong to me.
I think the whole idea of doing this in private memory is bad. The
visibility routines aren't written for that, and the above is just one
instance of that, and I am far from convinced it's the only one. So you
either need to work out how to rely on the visibility checking done in
cluster.c, or you need to modify rewriteheap.c to write via
shared_buffers.
I don't think we can change rewriteheap.c to use shared buffers --- that
code was written to do things in private memory for performance reasons
and I don't think setting hit bits justifies changing that.
I don't think that's necessarily contradictory. You'd need to use a
ringbuffer strategy for IO, like e.g. VACUUM does. But I admit it's not
a insignificant amount of work, and it might need some adjustements in
some places.
Can you be more specific about the cluster.c idea? I see
copy_heap_data() in cluster.c calling HeapTupleSatisfiesVacuum() with a
'buf' just like the code I am working in.
Yes, it does. But in contrast to your patch it does so on the *origin*
buffer. Which is
Re: [HACKERS] narwhal and PGDLLIMPORT
Andres Freund and...@2ndquadrant.com writes: On 2014-02-15 18:21:56 +0100, Andres Freund wrote: On 2014-02-15 12:16:58 -0500, Tom Lane wrote: Yeah, there are enough copies of that stuff that centralizing them sounds like a great idea. Call it pg_getopt.h, perhaps? Patch attached. I am not sure whether HAVE_GETOPT is the best condition to use, since it's set by configure by a link based check, same goes for HAVE_INT_OPTERR. The other choices would be relying on HAVE_GETOPT_H or a new AC_CHECK_DECL. Thanks. I'll look this over and try to get it committed before brolga's next scheduled run. At least this'll ensure we only have one place to tweak if more tweaking is needed. 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
Andres Freund and...@2ndquadrant.com writes: Patch attached. Committed with some cosmetic adjustments --- thanks! I am not sure whether HAVE_GETOPT is the best condition to use, since it's set by configure by a link based check, same goes for HAVE_INT_OPTERR. The other choices would be relying on HAVE_GETOPT_H or a new AC_CHECK_DECL. I'm pretty sure HAVE_GETOPT is *not* what we want to use for the variable declarations. I've tried HAVE_GETOPT_H for the moment, but that may need more tweaking depending on what the buildfarm has to say. I haven't touched entab.c because it's not linking with pgport, so there seems little use in changing it. Agreed. We don't have very high standards for its portability anyway. I've also removed some #ifndef WIN32's that didn't seem to make much sense. They might've been needed at one time, but we'll see. (I also took out a few includes of unistd.h that were clearly now redundant, though I didn't try to be thorough about it.) 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 2014-02-15 14:35:02 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Patch attached. Committed with some cosmetic adjustments --- thanks! I am not sure whether HAVE_GETOPT is the best condition to use, since it's set by configure by a link based check, same goes for HAVE_INT_OPTERR. The other choices would be relying on HAVE_GETOPT_H or a new AC_CHECK_DECL. I'm pretty sure HAVE_GETOPT is *not* what we want to use for the variable declarations. I've tried HAVE_GETOPT_H for the moment, but that may need more tweaking depending on what the buildfarm has to say. I only used it because it was what previously protected the getopt() declaration in port.h... But that should probably have been depending on !HAVE_GETOPT_H in the first place. 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] HBA files w/include support?
On Fri, Feb 14, 2014 at 11:10:48AM -0500, Tom Lane wrote: The argument about wanting to assemble a pg_hba file from separately managed configuration pieces seems to have some merit, but the weak spot there is how do you define the search order? Or are you planning to just cross your fingers and hope it doesn't matter too much? Well, in my case since the only auth method used is md5 the order really doesn't matter. Besides the point that each combination of dbname and username only appears once. But for a general use feature I can imagine it would be a concern. This is not an important feature for me though: the config file is generated by puppet with a bunch of loops and an include directory would not really reduce the amount of work. Have a nice day, -- Martijn van Oosterhout klep...@svana.org http://svana.org/kleptog/ He who writes carelessly confesses thereby at the very outset that he does not attach much importance to his own thoughts. -- Arthur Schopenhauer signature.asc Description: Digital signature
Re: [HACKERS] narwhal and PGDLLIMPORT
On 12/02/2014 17:39, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-02-12 11:26:56 -0500, Tom Lane wrote: Hm. So if we're giving up on the idea of ever getting rid of PGDLLIMPORT, we ought to actually remove that, so that the Cygwin build works more like the other Windows builds? Hm, I don't see a big advantage in detecting the errors as It won't hugely increase the buildfarm coverage. But --auto-import seems to require marking the .text sections as writable, avoiding that seems to be a good idea if we don't need it anymore. Given the rather small number of Windows machines in the buildfarm, I'd really like them all to be on the same page about what's valid and what isn't. Having to wait ~24 hours to find out if they're all happy with something isn't my idea of a good time. In any case, as long as we have discrepancies between them, I'm not going to be entirely convinced that any of them are telling the full truth. I'm going to go remove that switch and see if brolga starts failing. If it does, I'll be satisfied that we understand the issues here. regards, tom lane disabling is not working on cygwin gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard zic.o ialloc.o scheck.o localtime.o -L../../src/port -L../../src/common -Wl,--allow-multiple-definition -Wl,--disable-auto-import -L/usr/local/lib -Wl,--as-needed -lpgcommon -lpgport -lintl -lssl -lcrypto -lz -lreadline -lcrypt -o zic.exe zic.o:zic.c:(.text.startup+0xc9): undefined reference to `optarg' zic.o:zic.c:(.text.startup+0x10d): undefined reference to `optarg' /usr/lib/gcc/i686-pc-cygwin/4.8.2/../../../../i686-pc-cygwin/bin/ld: zic.o: bad reloc address 0x10d in section `.text.startup' /usr/lib/gcc/i686-pc-cygwin/4.8.2/../../../../i686-pc-cygwin/bin/ld: final link failed: Invalid operation collect2: error: ld returned 1 exit status just removing -Wl,--disable-auto-import and everything works $ gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard zic.o ialloc.o scheck.o localtime.o -L../../src/port -L../../src/common -Wl,--allow-multiple-definition -L/usr/local/lib -Wl,--as-needed -lpgcommon -lpgport -lintl -lssl -lcrypto -lz -lreadline -lcrypt -o zic.exe 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
Re: [HACKERS] narwhal and PGDLLIMPORT
On 2014-02-15 21:49:43 +0100, Marco Atzeri wrote: On 12/02/2014 17:39, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-02-12 11:26:56 -0500, Tom Lane wrote: Hm. So if we're giving up on the idea of ever getting rid of PGDLLIMPORT, we ought to actually remove that, so that the Cygwin build works more like the other Windows builds? Hm, I don't see a big advantage in detecting the errors as It won't hugely increase the buildfarm coverage. But --auto-import seems to require marking the .text sections as writable, avoiding that seems to be a good idea if we don't need it anymore. Given the rather small number of Windows machines in the buildfarm, I'd really like them all to be on the same page about what's valid and what isn't. Having to wait ~24 hours to find out if they're all happy with something isn't my idea of a good time. In any case, as long as we have discrepancies between them, I'm not going to be entirely convinced that any of them are telling the full truth. I'm going to go remove that switch and see if brolga starts failing. If it does, I'll be satisfied that we understand the issues here. regards, tom lane disabling is not working on cygwin gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard zic.o ialloc.o scheck.o localtime.o -L../../src/port -L../../src/common -Wl,--allow-multiple-definition -Wl,--disable-auto-import -L/usr/local/lib -Wl,--as-needed -lpgcommon -lpgport -lintl -lssl -lcrypto -lz -lreadline -lcrypt -o zic.exe zic.o:zic.c:(.text.startup+0xc9): undefined reference to `optarg' zic.o:zic.c:(.text.startup+0x10d): undefined reference to `optarg' /usr/lib/gcc/i686-pc-cygwin/4.8.2/../../../../i686-pc-cygwin/bin/ld: zic.o: bad reloc address 0x10d in section `.text.startup' /usr/lib/gcc/i686-pc-cygwin/4.8.2/../../../../i686-pc-cygwin/bin/ld: final link failed: Invalid operation collect2: error: ld returned 1 exit status just removing -Wl,--disable-auto-import and everything works $ gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard zic.o ialloc.o scheck.o localtime.o -L../../src/port -L../../src/common -Wl,--allow-multiple-definition -L/usr/local/lib -Wl,--as-needed -lpgcommon -lpgport -lintl -lssl -lcrypto -lz -lreadline -lcrypt -o zic.exe Please pull and retry, that already might fix it. The reason it's probably failing is the warnings about declspec you reported earlier. See 60ff2fdd9970ba29f5267317a5e7354d2658c1e5 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] narwhal and PGDLLIMPORT
On 15/02/2014 21:54, Andres Freund wrote: Please pull and retry, that already might fix it. The reason it's probably failing is the warnings about declspec you reported earlier. See 60ff2fdd9970ba29f5267317a5e7354d2658c1e5 Greetings, Andres Freund that is fine. there is a different one, later on [cut] ../../src/timezone/localtime.o ../../src/timezone/strftime.o ../../src/timezone/pgtz.o ../../src/port/libpgport_srv.a ../../src/common/libpgcommon_srv.a -lintl -lssl -lcrypto -lcrypt -lldap -o postgres libpq/auth.o:auth.c:(.text+0x1940): undefined reference to `in6addr_any' libpq/auth.o:auth.c:(.text+0x1954): undefined reference to `in6addr_any' libpq/auth.o:auth.c:(.text+0x196d): undefined reference to `in6addr_any' libpq/auth.o:auth.c:(.text+0x1979): undefined reference to `in6addr_any' /usr/lib/gcc/i686-pc-cygwin/4.8.2/../../../../i686-pc-cygwin/bin/ld: libpq/auth.o: bad reloc address 0xce4 in section `.rdata' collect2: error: ld returned 1 exit status Makefile:66: recipe for target 'postgres' failed make[2]: *** [postgres] Error 1 make[2]: Leaving directory '/pub/devel/postgresql/git/postgresql_build/src/backend' 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] OS X and ossp-uuid, next chapter
We already know that the uuid-ossp extension doesn't build OS X unless a
small patch is applied.
This has now gotten slightly worse after the Autoconf upgrade, because
it will now fail if a header is present but cannot be compiled.
(Previous versions would only warn. This is part of a decade-long
transition process Autoconf is doing.)
So now you get:
checking ossp/uuid.h usability... no
checking ossp/uuid.h presence... no
checking for ossp/uuid.h... no
checking uuid.h usability... no
checking uuid.h presence... yes
configure: WARNING: uuid.h: present but cannot be compiled
configure: WARNING: uuid.h: check for missing prerequisite headers?
configure: WARNING: uuid.h: see the Autoconf documentation
configure: WARNING: uuid.h: section Present But Cannot Be Compiled
configure: WARNING: uuid.h: proceeding with the compiler's result
configure: WARNING: ## ##
configure: WARNING: ## Report this to pgsql-b...@postgresql.org ##
configure: WARNING: ## ##
checking for uuid.h... no
configure: error: header file ossp/uuid.h or uuid.h is required for
OSSP-UUID
A possible patch is to hack up the uuid.h check to revert to the old
behavior; see attached.
I don't necessarily want to apply that, because it's an OS-specific hack
and it doesn't even work by itself unless we also patch the place where
the header is used (previously discussed). But I'll put it on record
here for future reporters and for the benefit of packagers.
diff --git a/configure b/configure
index 6ad165f..4476a46 100755
--- a/configure
+++ b/configure
@@ -1900,6 +1900,35 @@ $as_echo $ac_res 6; }
} # ac_fn_c_check_header_compile
+# ac_fn_c_check_header_preproc LINENO HEADER VAR
+# --
+# Tests whether HEADER is present, setting the cache variable VAR accordingly.
+ac_fn_c_check_header_preproc ()
+{
+ as_lineno=${as_lineno-$1} as_lineno_stack=as_lineno_stack=$as_lineno_stack
+ { $as_echo $as_me:${as_lineno-$LINENO}: checking for $2 5
+$as_echo_n checking for $2... 6; }
+if eval \${$3+:} false; then :
+ $as_echo_n (cached) 6
+else
+ cat confdefs.h - _ACEOF conftest.$ac_ext
+/* end confdefs.h. */
+#include $2
+_ACEOF
+if ac_fn_c_try_cpp $LINENO; then :
+ eval $3=yes
+else
+ eval $3=no
+fi
+rm -f conftest.err conftest.i conftest.$ac_ext
+fi
+eval ac_res=\$$3
+ { $as_echo $as_me:${as_lineno-$LINENO}: result: $ac_res 5
+$as_echo $ac_res 6; }
+ eval $as_lineno_stack; ${as_lineno_stack:+:} unset as_lineno
+
+} # ac_fn_c_check_header_preproc
+
# ac_fn_c_check_member LINENO AGGR MEMBER VAR INCLUDES
#
# Tries to find if the field MEMBER exists in type AGGR, after including
@@ -9400,7 +9429,7 @@ fi
if test $with_ossp_uuid = yes ; then
for ac_header in ossp/uuid.h
do :
- ac_fn_c_check_header_mongrel $LINENO ossp/uuid.h ac_cv_header_ossp_uuid_h $ac_includes_default
+ ac_fn_c_check_header_preproc $LINENO ossp/uuid.h ac_cv_header_ossp_uuid_h
if test x$ac_cv_header_ossp_uuid_h = xyes; then :
cat confdefs.h _ACEOF
#define HAVE_OSSP_UUID_H 1
@@ -9410,7 +9439,7 @@ else
for ac_header in uuid.h
do :
- ac_fn_c_check_header_mongrel $LINENO uuid.h ac_cv_header_uuid_h $ac_includes_default
+ ac_fn_c_check_header_preproc $LINENO uuid.h ac_cv_header_uuid_h
if test x$ac_cv_header_uuid_h = xyes; then :
cat confdefs.h _ACEOF
#define HAVE_UUID_H 1
diff --git a/configure.in b/configure.in
index aa23f9b..b1af8f9 100644
--- a/configure.in
+++ b/configure.in
@@ -1078,7 +1078,7 @@ fi
if test $with_ossp_uuid = yes ; then
AC_CHECK_HEADERS(ossp/uuid.h, [], [
AC_CHECK_HEADERS(uuid.h, [],
- [AC_MSG_ERROR([header file ossp/uuid.h or uuid.h is required for OSSP-UUID])])])
+ [AC_MSG_ERROR([header file ossp/uuid.h or uuid.h is required for OSSP-UUID])], [-])], [-])
fi
if test $PORTNAME = win32 ; then
--
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 2014-02-15 22:11:37 +0100, Marco Atzeri wrote: On 15/02/2014 21:54, Andres Freund wrote: Please pull and retry, that already might fix it. The reason it's probably failing is the warnings about declspec you reported earlier. See 60ff2fdd9970ba29f5267317a5e7354d2658c1e5 Greetings, Andres Freund that is fine. there is a different one, later on [cut] ../../src/timezone/localtime.o ../../src/timezone/strftime.o ../../src/timezone/pgtz.o ../../src/port/libpgport_srv.a ../../src/common/libpgcommon_srv.a -lintl -lssl -lcrypto -lcrypt -lldap -o postgres libpq/auth.o:auth.c:(.text+0x1940): undefined reference to `in6addr_any' Could you try additionally linking with -lwsock32? 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] narwhal and PGDLLIMPORT
Andres Freund and...@2ndquadrant.com writes: On 2014-02-15 22:11:37 +0100, Marco Atzeri wrote: ../../src/timezone/localtime.o ../../src/timezone/strftime.o ../../src/timezone/pgtz.o ../../src/port/libpgport_srv.a ../../src/common/libpgcommon_srv.a -lintl -lssl -lcrypto -lcrypt -lldap -o postgres libpq/auth.o:auth.c:(.text+0x1940): undefined reference to `in6addr_any' Could you try additionally linking with -lwsock32? The interesting question here is why it used to work. There is no extern for in6addr_any in our code, so there must have been a declaration of that constant in some system header. Which one, and what linkage is it defining, and where was the linkage getting resolved before? I notice that brolga is showing both this failure and some libxml issues. 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] gaussian distribution pgbench
Gaussian Pgbench v6 patch by Mitsumasa KONDO review patch v7.
* The purpose of the patch is to allow a pgbench script to draw from normally
distributed or exponentially distributed integer values instead of uniformly
distributed.
This is a valuable contribution to enable pgbench to generate more realistic
loads, which is seldom uniform in practice.
* Changes
I have updated the patch (v7) based on Mitsumasa latest v6:
- some code simplifications formula changes.
- I've added explicit looping probability computations in comments
to show the (low) looping probability of the iterative search.
- I've tried to clarify the sgml documentation.
- I've removed the 5.0 default value as it was not used anymore.
- I've renamed some variables to match the naming style around.
* Compilation
The patch applies and compiles against current head. It works as expected,
although there is few feedback from the script to show that. By looking
at the aid distribution in the pgbench_history table after a run, I
could check that the aid values are indeed skewed, depending on the
parameters.
* Mathematical soundness
I've checked again the mathematical soundness for the methods involved.
After further thoughts, I'm not that sure that there is not a bias induced
by taking the second value based on cos when the first based on sin
as failed the test. So I removed the cos computation for the gaussian version,
and simplified the code accordingly. This mean that it may be a little
less efficient, but I'm more confident that there is no bias.
* Conclusion
If Mitsumasa-san is okay with the changes I have made, I would suggest
to accept this patch.
--
Fabien.diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index 16b7ab5..afe4a32 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -106,6 +106,9 @@ extern int optind;
#define LOG_STEP_SECONDS 5 /* seconds between log messages */
#define DEFAULT_NXACTS 10 /* default nxacts */
+#define MIN_GAUSSIAN_THRESHOLD 2.0 /* minimum threshold for gauss */
+#define MIN_EXPONENTIAL_THRESHOLD 2.0 /* minimum threshold for exp */
+
int nxacts = 0; /* number of transactions per client */
int duration = 0; /* duration in seconds */
@@ -177,6 +180,14 @@ bool is_connect; /* establish connection for each transaction */
bool is_latencies; /* report per-command latencies */
int main_pid; /* main process id used in log filename */
+/* gaussian distribution tests: */
+double stdev_threshold; /* standard deviation threshold */
+booluse_gaussian = false;
+
+/* exponential distribution tests: */
+double exp_threshold; /* threshold for exponential */
+bool use_exponential = false;
+
char *pghost = ;
char *pgport = ;
char *login = NULL;
@@ -338,6 +349,88 @@ static char *select_only = {
SELECT abalance FROM pgbench_accounts WHERE aid = :aid;\n
};
+/* --exponential case */
+static char *exponential_tpc_b = {
+ \\set nbranches CppAsString2(nbranches) * :scale\n
+ \\set ntellers CppAsString2(ntellers) * :scale\n
+ \\set naccounts CppAsString2(naccounts) * :scale\n
+ \\setexponential aid 1 :naccounts :exp_threshold\n
+ \\setrandom bid 1 :nbranches\n
+ \\setrandom tid 1 :ntellers\n
+ \\setrandom delta -5000 5000\n
+ BEGIN;\n
+ UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;\n
+ SELECT abalance FROM pgbench_accounts WHERE aid = :aid;\n
+ UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;\n
+ UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;\n
+ INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);\n
+ END;\n
+};
+
+/* --exponential with -N case */
+static char *exponential_simple_update = {
+ \\set nbranches CppAsString2(nbranches) * :scale\n
+ \\set ntellers CppAsString2(ntellers) * :scale\n
+ \\set naccounts CppAsString2(naccounts) * :scale\n
+ \\setexponential aid 1 :naccounts :exp_threshold\n
+ \\setrandom bid 1 :nbranches\n
+ \\setrandom tid 1 :ntellers\n
+ \\setrandom delta -5000 5000\n
+ BEGIN;\n
+ UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;\n
+ SELECT abalance FROM pgbench_accounts WHERE aid = :aid;\n
+ INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);\n
+ END;\n
+};
+
+/* --exponential with -S case */
+static char *exponential_select_only = {
+ \\set naccounts CppAsString2(naccounts) * :scale\n
+ \\setexponential aid 1 :naccounts :exp_threshold\n
+ SELECT abalance FROM pgbench_accounts WHERE aid = :aid;\n
+};
+
+/* --gaussian case */
+static char *gaussian_tpc_b = {
+ \\set nbranches CppAsString2(nbranches) * :scale\n
+ \\set ntellers CppAsString2(ntellers) * :scale\n
+ \\set naccounts CppAsString2(naccounts) * :scale\n
+ \\setgaussian aid 1 :naccounts :stdev_threshold\n
+ \\setrandom bid 1 :nbranches\n
+ \\setrandom
Re: [HACKERS] narwhal and PGDLLIMPORT
On 2014-02-15 17:26:30 -0500, Tom Lane wrote:
Andres Freund and...@2ndquadrant.com writes:
On 2014-02-15 22:11:37 +0100, Marco Atzeri wrote:
../../src/timezone/localtime.o ../../src/timezone/strftime.o
../../src/timezone/pgtz.o ../../src/port/libpgport_srv.a
../../src/common/libpgcommon_srv.a -lintl -lssl -lcrypto -lcrypt -lldap -o
postgres
libpq/auth.o:auth.c:(.text+0x1940): undefined reference to `in6addr_any'
Could you try additionally linking with -lwsock32?
The interesting question here is why it used to work. There is no
extern for in6addr_any in our code, so there must have been a
declaration of that constant in some system header. Which one, and
what linkage is it defining, and where was the linkage getting
resolved before?
mingwcompat.c has the following ugly as heck tidbit:
#ifndef WIN32_ONLY_COMPILER
/*
* MingW defines an extern to this struct, but the actual struct isn't present
* in any library. It's trivial enough that we can safely define it
* ourselves.
*/
const struct in6_addr in6addr_any = {{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0}}};
I think when that was added the problem might just have been
misanalyzed, but due to the auto import magic this probably wasn't
noticed...
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] narwhal and PGDLLIMPORT
Marco Atzeri marco.atz...@gmail.com writes: 32 $ grep -rH in6addr_any * cygwin/in6.h:extern const struct in6_addr in6addr_any; cygwin/version.h: in6addr_any, in6addr_loopback. So how come there's a declspec on the getopt.h variables, but not this one? 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
Andres Freund and...@2ndquadrant.com writes:
On 2014-02-15 17:26:30 -0500, Tom Lane wrote:
The interesting question here is why it used to work. There is no
extern for in6addr_any in our code, so there must have been a
declaration of that constant in some system header. Which one, and
what linkage is it defining, and where was the linkage getting
resolved before?
mingwcompat.c has the following ugly as heck tidbit:
#ifndef WIN32_ONLY_COMPILER
/*
* MingW defines an extern to this struct, but the actual struct isn't present
* in any library. It's trivial enough that we can safely define it
* ourselves.
*/
const struct in6_addr in6addr_any = {{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0}}};
Yeah, I noticed that. AFAICS, mingwcompat.c isn't built in Cygwin builds,
so we'd probably need to put the constant someplace else entirely if we
end up defining it ourselves.
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 15/02/2014 23:37, Andres Freund wrote:
On 2014-02-15 17:26:30 -0500, Tom Lane wrote:
Andres Freund and...@2ndquadrant.com writes:
On 2014-02-15 22:11:37 +0100, Marco Atzeri wrote:
../../src/timezone/localtime.o ../../src/timezone/strftime.o
../../src/timezone/pgtz.o ../../src/port/libpgport_srv.a
../../src/common/libpgcommon_srv.a -lintl -lssl -lcrypto -lcrypt -lldap -o
postgres
libpq/auth.o:auth.c:(.text+0x1940): undefined reference to `in6addr_any'
Could you try additionally linking with -lwsock32?
The interesting question here is why it used to work. There is no
extern for in6addr_any in our code, so there must have been a
declaration of that constant in some system header. Which one, and
what linkage is it defining, and where was the linkage getting
resolved before?
mingwcompat.c has the following ugly as heck tidbit:
#ifndef WIN32_ONLY_COMPILER
/*
* MingW defines an extern to this struct, but the actual struct isn't present
* in any library. It's trivial enough that we can safely define it
* ourselves.
*/
const struct in6_addr in6addr_any = {{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0}}};
I think when that was added the problem might just have been
misanalyzed, but due to the auto import magic this probably wasn't
noticed...
Greetings,
Andres Freund
on cygwin header in /usr/include
32 $ grep -rH in6addr_any *
cygwin/in6.h:extern const struct in6_addr in6addr_any;
cygwin/version.h: in6addr_any, in6addr_loopback.
--
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.6.1
On Fri, Feb 14, 2014 at 4:55 AM, Andres Freund and...@2ndquadrant.com wrote: [ new patches ] 0001 already needs minor + * copied stuff from tuptoaster.c. Perhaps there should be toast_internal.h? Yes, please. If you can submit a separate patch creating this file and relocating this stuff there, I will commit it. + /* +* XXX: It's impolite to ignore our argument and keep decoding until the +* current position. +*/ Eh, what? +* We misuse the original meaning of SnapshotData's xip and subxip fields +* to make the more fitting for our needs. [...] +* XXX: Do we want extra fields instead of misusing existing ones instead? If we're going to do this, then it surely needs to be documented in snapshot.h. On the second question, you're not the first hacker to want to abuse the meanings of the existing fields; SnapshotDirty already does it. It's tempting to think we need a more principled approach to this, like what we've done with Node i.e. typedef enum ... SnapshotType; and then a separate struct definition for each kind, all beginning with SnapshotType type. + /* +* XXX: Timeline handling/naming. Do we need to include the timeline in +* snapshot's name? Outside of very obscure, user triggered, cases every +* LSN should correspond to exactly one timeline? +*/ I can't really comment intelligently on this, so you need to figure it out. My off-the-cuff thought is that erring on the side of including it couldn't hurt anything. + * XXX: use hex format for the LSN as well? Yes, please. + /* prepared abort contain a normal commit abort... */ contains. + /* +* Abort all transactions that we keep track of that are older +* than the record's oldestRunningXid. This is the most +* convenient spot for doing so since, in contrast to shutdown +* or end-of-recovery checkpoints, we have sufficient +* knowledge to deal with prepared transactions here. +*/ I have no real quibble with this, but I think the comment could be clarified slightly to state *what* knowledge we have here that we wouldn't have there. + /* only crash recovery/replication needs to care */ I believe I know what you're getting at here, but the next guy might not. I suggest: Although these records only exist to serve the needs of logical decoding, all the work happens as part of crash or archive recovery, so we don't need to do anything here. +* Treat HOT update as normal updates, there is no useful s/, t/. T/ +* There are cases in which inplace updates are used without xids +* assigned (like VACUUM), there are others (like CREATE INDEX +* CONCURRENTLY) where an xid is present. If an xid was assigned In-place updates can be used either by XID-bearing transactions (e.g. in CREATE INDEX CONCURRENTLY) or by XID-less transactions (e.g. VACUUM). In the former case, ... +* redundant because the commit will do that as well, but one +* we'll support decoding in-progress relations, this will be s/one/once/ s/we'll/we/ + /* we don't care about row level locks for now */ + case XLOG_HEAP_LOCK: + break; The position of the comment isn't consistent with the comments for the other WAL record type in this section; that is, it's above rather than below the case. +* transaction's contents as the various caches need to always be I think you should use since or because rather than as here, and maybe put a comma before it. +* the transaction's invalidations. This currently won't be needed if +* we're just skipping over the transaction, since that currently only +* happens when starting decoding, after we invalidated all caches, but +* transactions in other databases might have touched shared relations. I'm having trouble understanding what this means, especially the part after the but. + * Read a HeapTuple as WAL logged by heap_insert, heap_update and + * heap_delete, but not by heap_multi_insert into a tuplebuf. but not by heap_multi_insert needs punctuation both before and after. You can just add a comma after, or change it into a parenthetical phrase. As the above comments probably make clear, I'm pretty much happy with decode.c. + /* TODO: We got to change that someday soon.. */ Two periods. Maybe We need to change this some day soon. - and then follow that with a paragraph explaining what roughly what would need to be done. + /* shorter lines... */ + slot = MyReplicationSlot; + + /* first some sanity checks that are
Re: [HACKERS] narwhal and PGDLLIMPORT
On 2014-02-15 17:48:00 -0500, Tom Lane wrote: Marco Atzeri marco.atz...@gmail.com writes: 32 $ grep -rH in6addr_any * cygwin/in6.h:extern const struct in6_addr in6addr_any; cygwin/version.h: in6addr_any, in6addr_loopback. So how come there's a declspec on the getopt.h variables, but not this one? Well, those are real constants, so they probably are fully contained in the static link library, no need to dynamically resolve them. Note that the frontend libpq indeed links to wsock32, just as Mkvcbuild.pm does for both frontend and backend... It might be that ipv6 never worked for mingw and cygwin, and in6addr_any just resolved to some magic thunk --auto-import created... 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] narwhal and PGDLLIMPORT
Andres Freund and...@2ndquadrant.com writes: It might be that ipv6 never worked for mingw and cygwin, and in6addr_any just resolved to some magic thunk --auto-import created... Yeah, it would not be surprising if this exercise is exposing bugs we didn't even know we had. 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 2014-02-15 18:26:46 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: It might be that ipv6 never worked for mingw and cygwin, and in6addr_any just resolved to some magic thunk --auto-import created... Yeah, it would not be surprising if this exercise is exposing bugs we didn't even know we had. Agreed, but I think we should start fixing the missing PGDLLIMPORTs soon, at least the ones needed in the backbranches. AFAICT there's live bugs with postgres_fdw there. And possibly it might need more than one full cycle to get right... 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.6.1
On 2014-02-15 17:29:04 -0500, Robert Haas wrote:
On Fri, Feb 14, 2014 at 4:55 AM, Andres Freund and...@2ndquadrant.com wrote:
[ new patches ]
0001 already needs minor
Hm?
If there are conflicts, I'll push/send a rebased tomorrow or monday.
+* the transaction's invalidations. This currently won't be needed if
+* we're just skipping over the transaction, since that currently only
+* happens when starting decoding, after we invalidated all caches,
but
+* transactions in other databases might have touched shared
relations.
I'm having trouble understanding what this means, especially the part
after the but.
Let me rephrase, maybe that will help:
This currently won't be needed if we're just skipping over the
transaction because currenlty we only do so during startup, to get to
the first transaction the client needs. As we have reset the catalog
caches before starting to read WAL and we haven't yet touched any
catalogs there can't be anything to invalidate.
But if we're forgetting this commit because it's it happened in
another database, the invalidations might be important, because they
could be for shared catalogs and we might have loaded data into the
relevant syscaches.
Better?
+ if (IsTransactionState()
+ GetTopTransactionIdIfAny() != InvalidTransactionId)
+ ereport(ERROR,
+ (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+errmsg(cannot perform changeset
extraction in transaction that has performed writes)));
This is sort of an awkward restriction, as it makes it hard to compose
this feature with others. What underlies the restriction, can we get
rid of it, and if not can we include a comment here explaining why it
exists?
Well, you can write the result into a table if you're halfway
careful... :)
I think it should be fairly easy to relax the restriction to creating a
slot, but not getting data from it. Do you think that would that be
sufficient?
+ /* register output plugin name with slot */
+ strncpy(NameStr(slot-data.plugin), plugin,
+ NAMEDATALEN);
+ NameStr(slot-data.plugin)[NAMEDATALEN - 1] = '\0';
If it's safe to do this without a lock, I don't know why.
Well, the worst that could happen is that somebody else doing a SELECT *
FROM pg_replication_slot gets a incomplete plugin name... But we
certainly can hold the spinlock during it if you think that's better.
More broadly, I wonder why this is_init stuff is in here at all.
Maybe the stuff that happens in the is_init case should be done in the
caller, or another helper function.
The reason I moved it in there is that after the walsender patch there
are two callers and the stuff is sufficiently complex that I having it
twice lead to bugs.
The reason it's currenlty the same function is that sufficiently much of
the code would have to be shared that I found it it ugly to split. I'll
have a look whether I can figure something out.
+ /* prevent WAL removal as fast as possible */
+ ReplicationSlotsComputeRequiredLSN();
If there's a race here, can't we rejigger the order of operations to
eliminate it? Or is that just too hard and not worth it?
Yes, there's a small race which at the very least should be properly
documented.
Hm. Yes, I think we can plug the hole. If the race condition occurs we'd
take slightly longer to startup, which isn't bad. Will fix.
+begin_txn_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn)
+ state.callback_name = pg_decode_begin_txn;
+ ctx-callbacks.begin_cb(ctx, txn);
I feel that there's a certain lack of naming consistency between these
things. Can we improve that? (and similarly for parallel cases)
+pg_create_decoding_replication_slot(PG_FUNCTION_ARGS)
I thought we were going to have physical and logical slots, not
physical and decoding slots.
Ok.
+ /* make sure we don't end up with an unreleased slot */
+ PG_TRY();
+ {
...
+ PG_CATCH();
+ {
+ ReplicationSlotRelease();
+ ReplicationSlotDrop(NameStr(*name));
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
I don't think this is a very good idea. The problem with doing things
during error recovery that can themselves fail is that you'll lose the
original error, which is not cool, and maybe even blow out the error
stack. Many people have confuse PG_TRY()/PG_CATCH() with an
exception-handling system, but it's not. One way to fix this is to
put some of the initialization logic in ReplicationSlotCreate() just
prior to calling CreateSlotOnDisk(). If the work that needs to be
done is too complex or protracted to be done there, then I think that
it should be pulled out of the act of creating the replication slot
and made to happen as part of first use, or as a separate operation
Re: [HACKERS] [PATCH] Relocation of tablespaces in pg_basebackup
I've been working on your patch. Attached is a version I'd be happy to
commit. Please check that it's okay with you.
I rewrote the option argument parsing logic a little bit to be more
clear and provide more specific error messages.
I reinstated the requirement that both old and new directory are
absolute. After consideration, I think this makes sense because all
tablespace directories are always required to be absolute in other
contexts. (Note: Checking for absolute path by testing the first
character for '/' is not portable.)
I also removed the partial matching. This would have let -T /data1=...
also match /data11, which is clearly confusing. This logic would need
some intelligence about slashes, similar to fnmatch(). This could
perhaps be added later.
Finally, I wrote some test cases for this new functionality. See the
attached patch, which can be applied on top of
https://commitfest.postgresql.org/action/patch_view?id=1394.
From cc189020d04ff2311c92108620e4fc74f80c01c9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut pete...@gmx.net
Date: Sat, 15 Feb 2014 08:42:20 -0500
Subject: [PATCH] pg_basebackup: Add support for relocating tablespaces
Tablespaces can be relocated in plain backup mode by specifying one or
more -T olddir=newdir options.
From: Steeve Lennmark stee...@handeldsbanken.se
Reviewed-by: Peter Eisentraut pete...@gmx.net
---
doc/src/sgml/ref/pg_basebackup.sgml | 46 +-
src/bin/pg_basebackup/pg_basebackup.c | 166 +-
2 files changed, 204 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index c379df5..ea22331 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -203,6 +203,33 @@ titleOptions/title
/varlistentry
varlistentry
+ termoption-T replaceable class=parameterolddir/replaceable=replaceable class=parameternewdir/replaceable/option/term
+ termoption--tablespace-mapping=replaceable class=parameterolddir/replaceable=replaceable class=parameternewdir/replaceable/option/term
+ listitem
+ para
+Relocate the tablespace in directory replaceableolddir/replaceable
+to replaceablenewdir/replaceable during the backup. To be
+effective, replaceableolddir/replaceable must exactly match the
+path specification of the tablespace as it is currently defined. (But
+it is not an error if there is no tablespace
+in replaceableolddir/replaceable contained in the backup.)
+Both replaceableolddir/replaceable
+and replaceablenewdir/replaceable must be absolute paths. If a
+path happens to contain a literal=/literal sign, escape it with a
+backslash. This option can be specified multiple times for multiple
+tablespaces. See examples below.
+ /para
+
+ para
+If a tablespace is relocated in this way, the symbolic links inside
+the main data directory are updated to point to the new location. So
+the new data directory is ready to be used for a new server instance
+with all tablespaces in the updated locations.
+/para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption--xlogdir=replaceable class=parameterxlogdir/replaceable/option/term
listitem
para
@@ -528,9 +555,13 @@ titleNotes/title
/para
para
- The way productnamePostgreSQL/productname manages tablespaces, the path
- for all additional tablespaces must be identical whenever a backup is
- restored. The main data directory, however, is relocatable to any location.
+ Tablespaces will in plain format by default be backed up to the same path
+ they have on the server, unless the
+ option replaceable--tablespace-mapping/replaceable is used. Without
+ this option, running a plain format base backup on the same host as the
+ server will not work if tablespaces are in use, because the backup would
+ have to be written to the same directory locations as the original
+ tablespaces.
/para
para
@@ -570,6 +601,15 @@ titleExamples/title
(This command will fail if there are multiple tablespaces in the
database.)
/para
+
+ para
+ To create a backup of a local database where the tablespace in
+ filename/opt/ts/filename is relocated
+ to filename./backup/ts/filename:
+screen
+prompt$/prompt userinputpg_basebackup -D backup/data -T /opt/ts=$(pwd)/backup/ts/userinput
+/screen
+ /para
/refsect1
refsect1
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index b5682d6..b27678f 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -35,8 +35,24 @@
#include streamutil.h
+#define atooid(x) ((Oid) strtoul((x), NULL, 10))
+
+typedef struct TablespaceListCell
+{
+ struct TablespaceListCell *next;
+ char old_dir[MAXPGPATH];
+ char new_dir[MAXPGPATH];
+}
Re: [HACKERS] New hook after raw parsing, before analyze
On Sat, Feb 15, 2014 at 2:06 PM, David Beck db...@starschema.net wrote: - when the query arrives a smart rewrite would know 1) what tables are local 2) what tables need new catalog entries 3) what can be joined on the other side - the rewriter would potentially add SQL statements in the beginning of the query for creating the missing FDW catalog entries if needed - the FDW would be handled by the same extension so they can easily talk to each other about the status of the objects, so the rewriter would know if the background transfer of the small table is completed and should do the rewrite accordingly I think you have a pretty big impedence mismatch with Postgres which assumes the schema of the database is fairly static and known when parsing begins. To do what you describe you pretty much need to write your own SQL parser. There is a hook post_parse_analyze_hook but I think it comes too late as it comes after the analyze step which is when Postgres looks up the schema information for every relation mentioned in the query. What you would need is a post_parse_hook which would work on the raw parse tree before the analyze step. That doesn't seem terribly controversial to add though there may be some technical details. The API would of course be completely unstable from major release to major release -- the parse tree gets knocked around quite a bit. But that only gets you so far. I think you would be able to get lazy creation of schema objects fairly straightforwardly. Ie. Any legacy objects referenced in a query get corresponding fdw schema objects created before analyze is done -- though I would expect a few gotchas, possibly deadlocks, with concurrent queries creating the same objects. But I don't think that gets you the joins which I think would be quite a battle. And I have to wonder if you aren't going the long way around to do something that can be done more simply some other way. If you have 150k objects I wonder if your objects aren't all very similar and could be handled by a single Postgres schema object. Either a single FDW object or a simple function. If they really are different schema objects perhaps a single function that returns a more flexible data type like an hstore blob? That has obvious disadvantages over an object that the planner understands better but at least you would be in a well supported stable API (well, for interesting definitions of stable given 9.4 will have a whole new version of hstore). As a side note, you should evaluate carefully what lazily creating objects will buy you. Perhaps just creating 150k objects would be cheaper than maintaining this code. In particular since the user *might* access all 150k you still have to worry about the worst case anyway and it might be cheaper to just engineer for it in the first place. -- 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] old warning in docs
On Thu, Feb 13, 2014 at 10:55 AM, Bruce Momjian br...@momjian.us wrote: I have created the attached patch which removes many of the pre-8.0 references, and trims some of the 8.1-8.3 references. There are probably some of these that should be kept, but it is easier to show you all the possibilities and we can trim down the removal list based on feedback. The changes to lobj.sgml seem pointless. I would also vote for keeping xindex.sgml as-is. The rest of the changes seem like improvements. -- 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] old warning in docs
On Sat, Feb 15, 2014 at 07:19:46PM -0500, Robert Haas wrote: On Thu, Feb 13, 2014 at 10:55 AM, Bruce Momjian br...@momjian.us wrote: I have created the attached patch which removes many of the pre-8.0 references, and trims some of the 8.1-8.3 references. There are probably some of these that should be kept, but it is easier to show you all the possibilities and we can trim down the removal list based on feedback. The changes to lobj.sgml seem pointless. I would also vote for keeping xindex.sgml as-is. The rest of the changes seem like improvements. OK, done. Updated patch attached. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml new file mode 100644 index 30fd9bb..a5b74e6 *** a/doc/src/sgml/datatype.sgml --- b/doc/src/sgml/datatype.sgml *** NUMERIC *** 744,754 note para ! Prior to productnamePostgreSQL/productname 7.4, the precision in ! typefloat(replaceablep/replaceable)/type was taken to mean ! so many emphasisdecimal/ digits. This has been corrected to match the SQL ! standard, which specifies that the precision is measured in binary ! digits. The assumption that typereal/type and typedouble precision/type have exactly 24 and 53 bits in the mantissa respectively is correct for IEEE-standard floating point implementations. On non-IEEE platforms it might be off a little, but --- 744,750 note para ! The assumption that typereal/type and typedouble precision/type have exactly 24 and 53 bits in the mantissa respectively is correct for IEEE-standard floating point implementations. On non-IEEE platforms it might be off a little, but *** ALTER SEQUENCE replaceable class=param *** 844,859 /para /note - note - para - Prior to productnamePostgreSQL/productname 7.3, typeserial/type - implied literalUNIQUE/literal. This is no longer automatic. If - you wish a serial column to have a unique constraint or be a - primary key, it must now be specified, just like - any other data type. - /para - /note - para To insert the next value of the sequence into the typeserial/type column, specify that the typeserial/type --- 840,845 *** SELECT E'\\xDEADBEEF'; *** 1602,1609 The SQL standard requires that writing just typetimestamp/type be equivalent to typetimestamp without time zone/type, and productnamePostgreSQL/productname honors that ! behavior. (Releases prior to 7.3 treated it as typetimestamp ! with time zone/type.) typetimestamptz/type is accepted as an abbreviation for typetimestamp with time zone/type; this is a productnamePostgreSQL/productname extension. /para --- 1588,1594 The SQL standard requires that writing just typetimestamp/type be equivalent to typetimestamp without time zone/type, and productnamePostgreSQL/productname honors that ! behavior. typetimestamptz/type is accepted as an abbreviation for typetimestamp with time zone/type; this is a productnamePostgreSQL/productname extension. /para diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml new file mode 100644 index bae2e97..8ace8bd *** a/doc/src/sgml/ddl.sgml --- b/doc/src/sgml/ddl.sgml *** CREATE TABLE circles ( *** 1106,1114 within a single transaction. In practice this limit is not a problem mdash; note that the limit is on the number of acronymSQL/acronym commands, not the number of rows processed. ! Also, as of productnamePostgreSQL/productname 8.3, only commands ! that actually modify the database contents will consume a command ! identifier. /para /sect1 --- 1106,1113 within a single transaction. In practice this limit is not a problem mdash; note that the limit is on the number of acronymSQL/acronym commands, not the number of rows processed. ! Also, only commands that actually modify the database contents will ! consume a command identifier. /para /sect1 *** REVOKE CREATE ON SCHEMA public FROM PUBL *** 1873,1883 /para para ! In productnamePostgreSQL/productname versions before 7.3, ! table names beginning with literalpg_/ were reserved. This is ! no longer true: you can create such a table name if you wish, in ! any non-system schema. However, it's best to continue to avoid ! such names, to ensure that you won't suffer a conflict if some future version defines a system table named the same as your table. (With the default search path, an unqualified reference to your table name would then be
Re: [HACKERS] 9.2.1 index-only scans : abnormal heap fetches after VACUUM FULL
On Sat, Feb 15, 2014 at 07:08:40PM +0100, Andres Freund wrote: Can you be more specific about the cluster.c idea? I see copy_heap_data() in cluster.c calling HeapTupleSatisfiesVacuum() with a 'buf' just like the code I am working in. Yes, it does. But in contrast to your patch it does so on the *origin* buffer. Which is in shared memory. The idea would be to modify rewrite_heap_tuple() to get a new parameter informing it it about the tuple's visibility. That'd allow you to avoid doing any additional visibility checks. Unfortunately that would still not fix the problem that visibilitymap_set requires a real buffer to be passed in. If you're going that route, you'll need to make a bit more sweeping changes. You'd need to add new blockno parameter and a BufferIsValid() check to the right places in log_heap_visible(). Pretty ugly if you ask me... Thank you for the thorough review. Unless someone else can complete this, I think it should be marked as returned with feedback. I don't think I am going to learn enough to complete this during the commit-fest. Since learning of the limitations in setting vmmap bits for index-only scans in October, I have been unable to improve VACUUM/CLUSTER, and unable to improve autovacuum --- a double failure. I guess there is always PG 9.5. -- 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
(2014/02/15 2:32), Tom Lane wrote: I wrote: Hiroshi Inoue in...@tpf.co.jp writes: One thing I'm wondering about is that plperl is linking perlxx.lib not libperlxx.a. I made a patch following plpython and it also works here. Is it worth trying? I hadn't noticed that part of plpython's Makefile before. Man, that's an ugly technique :-(. Still, there's little about this platform that isn't ugly. Let's try it and see what happens. And what happens is this: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=narwhaldt=2014-02-14%2017%3A00%3A02 namely, it gets through plperl now and then chokes with the same symptoms on pltcl. So I guess we need the same hack in pltcl. The fun never stops ... Pltcl still fails. tclxx.dll lives in bin directory not in lib directory. The attached patch would fix the problem. regards, Hiroshi Inoue diff --git a/src/pl/tcl/Makefile b/src/pl/tcl/Makefile index e0fb13e..2ab2a27 100644 --- a/src/pl/tcl/Makefile +++ b/src/pl/tcl/Makefile @@ -53,7 +53,7 @@ PSQLDIR = $(bindir) ifeq ($(PORTNAME), win32) tclwithver = $(subst -l,,$(filter -l%, $(TCL_LIB_SPEC))) -TCLDLL = $(subst -L,,$(filter -L%, $(TCL_LIB_SPEC)))/$(tclwithver).dll +TCLDLL = $(dir $(TCLSH))/$(tclwithver).dll OBJS += lib$(tclwithver).a -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
