Re: [HACKERS] WAL consistency check facility
On Sat, Sep 10, 2016 at 8:33 PM, Robert Haas wrote: > On Sat, Sep 10, 2016 at 3:19 AM, Michael Paquier > wrote: >> On Fri, Sep 9, 2016 at 4:01 PM, Kuntal Ghosh >> wrote: > - If WAL consistency check is enabled for a rmgrID, we always include > the backup image in the WAL record. What happens if wal_consistency has different settings on a standby and its master? If for example it is set to 'all' on the standby, and 'none' on the master, or vice-versa, how do things react? An update of this parameter should be WAL-logged, no? >>> It is possible to set wal_consistency to 'All' in master and any other >>> values in standby. But, the scenario you mentioned will cause error in >>> standby since it may not get the required backup image for wal >>> consistency check. I think that user should be responsible to set >>> this value correctly. We can improve the error message to make the >>> user aware of the situation. >> >> Let's be careful here. You should as well consider things from the >> angle that some parameter updates are WAL-logged as well, like >> wal_level with the WAL record XLOG_PARAMETER_CHANGE. > > It seems entirely unnecessary for the master and the standby to agree > here. I think what we need is two GUCs. One of them, which affects > only the master, controls whether the validation information is > including in the WAL, and the other, which affects only the standby, > affects whether validation is performed when the necessary information > is present. > I think from the clarity perspective, this option sounds good, but I am slightly afraid that it might be inconvenient for users to set the different values for these two parameters. > Or maybe skip the second one and just decree that > standbys will always validate if the necessary information is present. Sounds like a better alternative and probably easier to configure for users. -- 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] WAL consistency check facility
On Sat, Sep 10, 2016 at 12:49 PM, Michael Paquier wrote: > On Fri, Sep 9, 2016 at 4:01 PM, Kuntal Ghosh > wrote: > - In recovery tests (src/test/recovery/t), I've added wal_consistency parameter in the existing scripts. This feature doesn't change the expected output. If there is any inconsistency, it can be verified in corresponding log file. >>> >>> I am afraid that just generating a WARNING message is going to be >>> useless for the buildfarm. If we want to detect errors, we could for >>> example have an additional GUC to trigger an ERROR or a FATAL, taking >>> down the cluster, and allowing things to show in red on a platform. >>> >> Yes, we can include an additional GUC to trigger an ERROR for any >> inconsistency. > > I'd like to hear extra opinions about that, but IMO just having an > ERROR would be fine for the first implementation. Once you've bumped > into an ERROR, you are likely going to fix it first. > +1 for just an ERROR to detect the inconsistency. I think adding additional GUC just to raise error level doesn't seem to be advisable. -- 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] pg_sequence catalog
On Sun, Sep 11, 2016 at 12:39 AM, Andres Freund wrote: > On 2016-09-10 17:23:21 +0530, Amit Kapila wrote: >> > >> >> I may be missing something here, but why would it contend on a lock, >> as per locking scheme proposed by Alvaro, access to sequence object >> will need a share lock on buffer page. > > To make checkpointing/bgwriter work correctly we need exclusive locks on > pages while writing..., or some new lock level preventing the page from > being written out, while "shared dirtying" locks are being held. > Right and I think you have a very valid concern, but if we think that storing multiple sequences on a same page is a reasonable approach, then we can invent some locking mechanism as indicated by you such that two writes on same page won't block each other, but they will be blocked with bgwriter/checkpointer. -- 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] Write Ahead Logging for Hash Indexes
On Sun, Sep 11, 2016 at 4:10 AM, Mark Kirkwood wrote: > > > performed several 10 hour runs on size 100 database using 32 and 64 clients. > For the last run I rebuilt with assertions enabled. No hangs or assertion > failures. > Thanks for verification. Do you think we can do some read-only workload benchmarking using this server? If yes, then probably you can use concurrent hash index patch [1] and cache the metapage patch [2] (I think Mithun needs to rebase his patch) to do so. [1] - https://www.postgresql.org/message-id/caa4ek1j6b8o4pcepqrxnyblvbftonmjeem+qn0jzx31-obx...@mail.gmail.com [2] - https://www.postgresql.org/message-id/cad__ouhj29cebif_flge4t9vj_-cfxbwcxhjo+d_16txbem...@mail.gmail.com -- 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] sequence data type
On 9/10/16, Jim Nasby wrote: > On 9/3/16 6:01 PM, Gavin Flower wrote: >> I am curious as to the use cases for other possibilities. > > A hex or base64 type might be interesting, should those ever get created... > -- > Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Hex or base64 are not data types. They are just different representation types of binary sequences. Even for bigints these representations are done after writing numbers as byte sequences. -- Best regards, Vitaly Burovoy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Override compile time log levels of specific messages/modules
On 9/6/16 5:18 AM, Craig Ringer wrote: I think something automatic that we clearly define as unstable and not to be relied upon would be preferable. Plus we already have much of the infrastructure in elog.c as used by errcontext etc. Actually, I wish this was a straight-up logging level feature, because I need it all the time when debugging complicated user-level code. Specifically, I wish there was a GUC that would alter (client|log)_min_messages upon entering a specific function, either for just that function or for anything that function subsequently called. Bonus points if you could also specify a regex that the message text had to match. I realize that code-wise that's completely incompatible with what you're discussing here, but I think the same API could be used (maybe as a different GUC). -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sequence data type
On 9/3/16 6:01 PM, Gavin Flower wrote: I am curious as to the use cases for other possibilities. A hex or base64 type might be interesting, should those ever get created... -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- Sent 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] Generic type subscription
On 9/9/16 6:29 AM, Dmitry Dolgov wrote: Regarding to the previous conversations [1], [2], here is a patch (with some improvements from Nikita Glukhov) for the generic type subscription. Awesome! Please make sure to add it to the Commit Fest app. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
On 8/1/16 11:38 AM, Bruce Momjian wrote: I am hoping for a "novice" mode that issues warnings about possible bugs, e.g. unintentionally-correlated subselect, and this could be part of that. Somewhat related; I've recently been wondering about a mode that disallows Const's in queries coming from specific roles. The idea there is to make it impossible for an application to pass a constant in, which would make it impossible for SQL injection to happen. With how magical modern frameworks/languages are, it's often impossible to enforce that at the application layer. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- Sent 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] Alter or rename enum value
On 9/8/16 4:55 AM, Emre Hasegeli wrote: The main problem that has been discussed before was the indexes. One way is to tackle with it is to reindex all the tables after the operation. Currently we are doing it when the datatype of indexed columns change. So it should be possible, but very expensive. Why not just disallow dropping a value that's still in use? That's certainly what I would prefer to happen by default... -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Merge Join with an Index Optimization
Hi, As I understand it, a merge join will currently read all tuples from both subqueries (besides early termination). I believe it should be possible to take advantages of the indexes on one or both of the tables being read from to skip a large number of tuples that would currently be read. As an example, let's say we are doing equi merge join between two tables with the following data: (3, 4, 5, 9, 10, 11) (0, 1, 2, 6, 7, 8) Currently, even though no tuples would be returned from the merge join, all of the data will be read from both tables. If there are indexes on both tables, pg should be able to execute as follows. Get the tuple with value 3 from the first table and then look up the first value greater than 3 in the second table using the index. In this case, that value would be 6. Since 3 != 6, pg would then look up the lowest value greater than 6 in the first table. This process repeats, and tuples are output whenever the values are equal. This can be thought of as an alternating nested loop join, where the positions in the indexes are maintained. In the case where only one of the tables has an index, this can be thought of as a nested loop join where the inner relation is the table with the index, and the position in that index is maintained while iterating over the outer relation (the outer relation would need to be sorted for this to work). I can't demonstrate in any benchmarks, but I imagine this would dramatically speed up cases of a merge join between two tables where the number of tuples that satisfy the join condition is small. I don't know the Postgres internals well enough to know if there is anything obvious that would prevent this optimization. Thanks, Michael
Re: [HACKERS] feature request: explain "with details" option
On 9/8/16 11:35 PM, Tom Lane wrote: This isn't simple because there are often *lots* of variants. You > don't just want to see the "top 10" candidate plans, because they're > probably a bunch of small variants on the same plan; the ones you'll > be interested in will probably be very different plans with very bad > relative estimates. The other big problem here is that the planner tries *very* hard to reject losing paths early; so it does not even form an explainable plan for a large fraction of the search space. (And if it did, you'd be dead before you got your EXPLAIN result back.) What I've wished for is the ability to see plans that were close in cost to the best case scenario, since that indicates that a slight change in statistics would push the planner in another direction (sometimes with disastrous results). Maybe allowing some number of plans to bubble up if they were within X percent of the winner wouldn't be that horrible. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Let file_fdw access COPY FROM PROGRAM
V2 of this patch: Changes: * rebased to most recent master * removed non-tap test that assumed the existence of Unix sed program * added non-tap test that assumes the existence of perl * switched from filename/program to filename/is_program to more closely follow patterns in copy.c * slight wording change in C comments On Thu, Sep 8, 2016 at 6:59 PM, Craig Ringer wrote: > On 9 Sep. 2016 03:45, "Corey Huinker" wrote: > > > > > > > Stylistically, would a separate .pl file for the emitter be preferable > to something inline like > > > >> perl -e 'print "a\tb\tcc\t4\n"; print "b\tc\tdd\t5\n"' > > I'd be fine with that and a suitable comment. Just be careful with > different platforms' shell escaping rules. > diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index b471991..bf9753a 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -59,6 +59,7 @@ struct FileFdwOption static const struct FileFdwOption valid_options[] = { /* File options */ {"filename", ForeignTableRelationId}, + {"program", ForeignTableRelationId}, /* Format options */ /* oids option is not supported */ @@ -85,10 +86,11 @@ static const struct FileFdwOption valid_options[] = { */ typedef struct FileFdwPlanState { - char *filename; /* file to read */ - List *options;/* merged COPY options, excluding filename */ - BlockNumber pages; /* estimate of file's physical size */ - double ntuples;/* estimate of number of rows in file */ + char *filename; /* file/program to read */ + boolis_program; /* true if filename represents an OS command */ + List *options;/* merged COPY options, excluding filename and program */ + BlockNumber pages; /* estimate of file or program output's physical size */ + double ntuples;/* estimate of number of rows in file/program output */ } FileFdwPlanState; /* @@ -96,9 +98,10 @@ typedef struct FileFdwPlanState */ typedef struct FileFdwExecutionState { - char *filename; /* file to read */ - List *options;/* merged COPY options, excluding filename */ - CopyState cstate; /* state of reading file */ + char *filename; /* file/program to read */ + boolis_program; /* true if filename represents an OS command */ + List *options;/* merged COPY options, excluding filename and is_program */ + CopyState cstate; /* state of reading file or program */ } FileFdwExecutionState; /* @@ -139,7 +142,9 @@ static bool fileIsForeignScanParallelSafe(PlannerInfo *root, RelOptInfo *rel, */ static bool is_valid_option(const char *option, Oid context); static void fileGetOptions(Oid foreigntableid, - char **filename, List **other_options); + char **filename, + bool *is_program, + List **other_options); static List *get_file_fdw_attribute_options(Oid relid); static bool check_selective_binary_conversion(RelOptInfo *baserel, Oid foreigntableid, @@ -196,16 +201,17 @@ file_fdw_validator(PG_FUNCTION_ARGS) /* * Only superusers are allowed to set options of a file_fdw foreign table. -* This is because the filename is one of those options, and we don't want -* non-superusers to be able to determine which file gets read. +* The reason for this is to prevent non-superusers from changing the +* definition to access an arbitrary file not visible to that user +* or to run programs not accessible to that user. * * Putting this sort of permissions check in a validator is a bit of a * crock, but there doesn't seem to be any other place that can enforce * the check more cleanly. * -* Note that the valid_options[] array disallows setting filename at any -* options level other than foreign table --- otherwise there'd still be a -* security hole. +* Note that the valid_options[] array disallows setting filename and +* program at any options level other than foreign table --- otherwise +* there'd still be a security hole. */ if (catalog == ForeignTableRelationId && !superuser()) ereport(ERROR, @@ -247,11 +253,11 @@ file_fdw_validator(PG_FUNCTION_ARGS) } /* -* Separate out filename and column-specific options, since +* Separate out filename, program, and c
Re: [HACKERS] Write Ahead Logging for Hash Indexes
On 09/09/16 14:50, Mark Kirkwood wrote: Yeah, good suggestion about replacing (essentially) all the indexes with hash ones and testing. I did some short runs with this type of schema yesterday (actually to get a feel for if hash performance vs btree was compareable - does seem tp be) - but probably longer ones with higher concurrency (as high as I can manage on a single socket i7 anyway) is a good plan. If Ashutosh has access to seriously large numbers of cores then that is even better :-) I managed to find a slightly bigger server (used our openstack cloud to run a 16 cpu vm). With the schema modified as follows: bench=# \d pgbench_accounts Table "public.pgbench_accounts" Column | Type | Modifiers --+---+--- aid | integer | not null bid | integer | abalance | integer | filler | character(84) | Indexes: "pgbench_accounts_pkey" hash (aid) bench=# \d pgbench_branches Table "public.pgbench_branches" Column | Type | Modifiers --+---+--- bid | integer | not null bbalance | integer | filler | character(88) | Indexes: "pgbench_branches_pkey" hash (bid) bench=# \d pgbench_tellers Table "public.pgbench_tellers" Column | Type | Modifiers --+---+--- tid | integer | not null bid | integer | tbalance | integer | filler | character(84) | Indexes: "pgbench_tellers_pkey" hash (tid) bench=# \d pgbench_history Table "public.pgbench_history" Column |Type | Modifiers +-+--- tid| integer | bid| integer | aid| integer | delta | integer | mtime | timestamp without time zone | filler | character(22) | Indexes: "pgbench_history_pkey" hash (bid) performed several 10 hour runs on size 100 database using 32 and 64 clients. For the last run I rebuilt with assertions enabled. No hangs or assertion failures. regards Mark -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] C++ port of Postgres
Christian Convey writes: >If that's correct, then it sounds like the only way Joy's commit has >a chance of acceptance is if Peter's commit is rejected. >Because Peter's commit might be merged as part of the 2016-09 >commitfest, but Joy's can show up until 2016-11 at the earliest. No, we're not that rigid about it. The commitfest mechanism is really mostly meant to ensure that every submission does get looked at within a reasonable amount of time and doesn't get forgotten about. (We do tend to be willing to tell new patches they've got to wait till the next fest, but that's mainly when we're already feeling overloaded by what's in the current queue.) In this case it would be nonsensical to consider only Peter's submission and not Joy's, because part of the issue is whether one's better than the other. > There seems to be a little ambiguity regarding the exact version of > the code to be reviewed. This is true for both Joy's and Peter's > submissions: >* Joy's email provides a link to a Github repo, but does not specify > a particular commit (or even branch) in that repo: [2] >* In the email thread, Peter did provide a patch set: [3] > but the corresponding commitfest entry references a github branch: [4] Well, at the point we're at here, it's probably not that important exactly which version of a patchset you're looking at; I'd take the latest. > Q4) Do we require that any submitted patches appear as attachments on > the pgsql-hackers email list, or is a github URL good enough? Actually, we *do* require that, mainly as a formality to show that the author intended to submit it for inclusion in Postgres. (We don't want people coming back later and saying "hey, you ripped off this code from my github repo without my permission".) If we were seriously considering adopting Joy's version then that would have to happen before anything got merged. But at this point it seems like we're in a very exploratory phase and there's no need for legal niceties yet. > Q5) (This question is more generic.) I'm accustomed to using Github's > pull-request system, where I can engage in dialog regarding specifc > lines of a patch. I haven't noticed anything similar being used for > PG code reviews, but perhaps I'm just looking in the wrong places. > Are all PG code reviews basically just back-and-forth email > conversations on the pgsql-hackers list? Yup, that's how we roll ... it's ancient habit from before there was such a thing as git, let alone github, but we've not seen fit to change. It does have the advantage that it's about equally amenable to high- level discussions and line-by-line 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] [GENERAL] C++ port of Postgres
Hi Heikki, Could I ask you a newbie-reviewer question about something I'm seeing here? https://commitfest.postgresql.org/10/776/ >From some reading I've done (e.g., Stephen Frost's PGCon 2011 slides), I got the impression that a successful patch would always have this sequence of states in commitfest: 1. patch-record created ... 2. Needs Review ... 3. Ready for Committer But if I'm reading the patch's activity log correctly, it looks like you marked the patch as "Ready for Committer" (2016-09-06 18:59:02) without any record of it having been reviewed. Was that intentional? Thanks very much, Christian P.S. I'm asking because I was planning to review that patch. But I can't tell if any more review by a non-committer is still required by the commitfest workflow. Kind regards, Christian On Tue, Sep 6, 2016 at 3:15 PM, Christian Convey wrote: > On Tue, Sep 6, 2016 at 3:12 PM, Tom Lane wrote: >>> (2) It seems like there are still a few big questions about this commit: >>>- Is it wanted at the moment? It didn't seem like there's a >>> consensus about whether or not this enhancement should be >>> merged, even if the patch is pretty minimal. >>>- It seems like there are two competing patch >>> sets in play for this enhancement: Joy's and >>> Peter's. Presumably at most one of them would >>> be merged. >> >> These are things that reviews should be helping to decide. It's probably >> a squishier topic than some patches, but if you're interested, feel free >> to read code and weigh in. > > Thanks. It sounds like worst-case scenario, I perform an unneeded > review. I'll give it a shot. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] High-CPU consumption on information_schema (only) query
> > > Without having at least compared EXPLAIN outputs from the two boxes, you > have no business jumping to that conclusion. > > If EXPLAIN does show different plans, my first instinct would be to wonder > whether the pg_stats data is equally up-to-date on both boxes. > > regards, tom lane > Thanks. EXPLAIN plans were different but (don't have them now and) didn't know system catalogs were so severely affected by outdated statistics as well (which is why I was looking for any other reasons I might be missing). I reckon an ANALYSE; should solve this? ... Would get back if I have something else to offer. - robins -- - robins
Re: [HACKERS] [GENERAL] C++ port of Postgres
> Thanks. It sounds like worst-case scenario, I perform an unneeded > review. I'll give it a shot. Hi guys, Apologies for more boring process-related questions, but any pointers would be greatly appreciated... I'm a bit confused about how PG's code-review process is meant to handle this C++ port. My confusion may stem from the combination of my inexperience with the process, and there being two competing patch sets. Here's some background: * My intention was to review Joy's patch. * On "commitfest.postgresql.org" (for 2016-09), the only C++ -related patch I found was Peter's: [1] * I wrongly assumed that the commitfest entry would be for Joy's patch, not Peter's, so I signed up as its reviewer. (That's fine - I don't mind reviewing both authors' patch sets.) But here are my questions: Q1) My understanding of PG's code-review process is that it's a pipeline: Step 1. The discussion starts on the pgsql-hackers mailing list, where the author posts a patch. He/she may also post revised patches based on the discussion. Step 2. A subset of those discussions are modeled by new entries in the commitfest website. Step 3. A subset of those commitfest items get merged. If that's correct, then it sounds like the only way Joy's commit has a chance of acceptance is if Peter's commit is rejected. Because Peter's commit might be merged as part of the 2016-09 commitfest, but Joy's can show up until 2016-11 at the earliest. Is my understanding correct? There seems to be a little ambiguity regarding the exact version of the code to be reviewed. This is true for both Joy's and Peter's submissions: * Joy's email provides a link to a Github repo, but does not specify a particular commit (or even branch) in that repo: [2] * In the email thread, Peter did provide a patch set: [3] but the corresponding commitfest entry references a github branch: [4] So I have a few questions here: Q2) Are authors expected to submit an unambiguous patch to frame the discussion? (I.e,. a specific patch file, or a specific git commit hash, as opposed to a github repo or a github branch.) Q3) Are authors expected to submit a single patch/commit, or is it acceptable / desirable for a large patch to be broken up as Peter has done? Q4) Do we require that any submitted patches appear as attachments on the pgsql-hackers email list, or is a github URL good enough? Q5) (This question is more generic.) I'm accustomed to using Github's pull-request system, where I can engage in dialog regarding specifc lines of a patch. I haven't noticed anything similar being used for PG code reviews, but perhaps I'm just looking in the wrong places. Are all PG code reviews basically just back-and-forth email conversations on the pgsql-hackers list? Thanks, Christian [1] https://commitfest.postgresql.org/10/776/ [2] https://www.postgresql.org/message-id/CABgyVxDBd3EvRdo-Rd6eo8QPEqV8%3DShaU2SJfo16wfE0R-hXTA%40mail.gmail.com [3] https://www.postgresql.org/message-id/bf9de63c-b669-4b8c-d33b-4a5ed11cd5d4%402ndquadrant.com [4] https://github.com/petere/postgresql/tree/commitfest/c%2B%2B -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] High-CPU consumption on information_schema (only) query
Robins Tharakan writes: > I completely agree. With 'irrelevant' I was only trying to imply that > irrespective of the complexity of the query, a replicated box was seeing > similar slowness whereas a Restored DB wasn't. It felt that the SQL itself > isn't to blame here... Without having at least compared EXPLAIN outputs from the two boxes, you have no business jumping to that conclusion. If EXPLAIN does show different plans, my first instinct would be to wonder whether the pg_stats data is equally up-to-date on both boxes. 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] High-CPU consumption on information_schema (only) query
On Fri, 9 Sep 2016 at 09:39 Andres Freund wrote: > On 2016-09-07 23:37:31 +, Robins Tharakan wrote: > > If someone asks for I could provide SQL + EXPLAIN, but it feels > irrelevant > > here. > > Why is that? information_schema are normal sql queries, and some of them > far from trivial. > > Andres > Hi Andres, I completely agree. With 'irrelevant' I was only trying to imply that irrespective of the complexity of the query, a replicated box was seeing similar slowness whereas a Restored DB wasn't. It felt that the SQL itself isn't to blame here... In effect, I was trying to ask if I am forgetting / missing something very obvious / important that could cause such an observation. As others recommended, I am unable to have direct access to the production (master / slave) instances and so GDB / stack trace options are out of bounds at this time. I'll revert if I am able to do that. - thanks robins -- - robins
Re: [HACKERS] Re: [COMMITTERS] pgsql: Use LEFT JOINs in some system views in case referenced row doesn
I wrote: > I looked into this a little. There are at least three things we could > do here: > 1. Decide that showing walsenders is a good thing. I'm not really > sure why it isn't -- for example, we could take the trouble to display > the current replication command as the walsender's activity. > 2. Suppress walsenders by the expedient of ignoring PGPROCs that have > zero database ID. This would also ignore other process types that aren't > connected to a specific database. This is pretty backwards-compatible > because it's equivalent to what used to happen implicitly via the inner > join in the view. > 3. Suppress walsenders by adding some new field to PgBackendStatus that > identifies them, and having pg_stat_get_activity() use that to filter. BTW, I had been thinking we could implement #2 or #3 relatively painlessly by adding filter logic inside pg_stat_get_activity(). However, I now notice that that function is also used by the pg_stat_replication view, which joins its output to pg_stat_get_wal_senders(). That means we can *not* simply suppress walsenders in the function's output; if we want to filter them away in pg_stat_activity, we have to change the view definition somehow, and probably the function's API as well. And that means we can't change the behavior without an initdb, which is kind of annoying to have to do after rc1. The fact that the pg_stat_replication view does show walsender processes seems like possibly a reasonable argument for not showing them in pg_stat_activity. But we'd have to do some rejiggering of the view definition to make that happen. Don't have a strong opinion which way it ought to work --- I could go with either the "walsenders are a perfectly good activity" position or the "they should appear only in pg_stat_replication" one. But the latter will take an initdb to do, and I'm inclined to the position that it's too late for that in 9.6. As I noted, it was already inconsistent for per-db walsenders long before 9.6, without field complaints, so I'm having a hard time seeing this as a critical bug. 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] feature request: explain "with details" option
On Thu, Sep 8, 2016 at 10:40 AM, Roger Pack wrote: > My apologies if this was already requested before... > > I think it would be fantastic if postgres had an "explain the explain" > option: > Today's explain tells us what loops and scans were used, and relative > costs, etc. It doesn't seem to tell *why* the planner elected to use > what it did. > > For instance, in the case of a corrupted index, it doesn't say why > it's not using that index, it just doesn't use it, causing some > confusion to end users. At least causing confusion to me. > I've never seen such a thing. If an index is corrupt, it still gets used like normal. You just get wrong results, or crashes, depending on the nature of the corruption. Or in the case of where it iterates over an entire table (seq. scan) > instead of using an index because the index range specified "is most > of the table" (thus not helpful to use the index) The planner just comes up with plans that use a seq scan, and plans that use an index; and then compares the cost of them and finds that one cost is lower than the other. It never explicitly develops a specific notion of "I won't use the index because I'm retrieving too much of the table". So it wouldn't be just a matter of reporting something that isn't currently reported, it would first have to infer that thing in the first place, and that would probably be very hard. > ...The choice is > appropriate. The reasoning why is not explicitly mentioned. Again > causing possibility for some delay as you try to "decipher the mind" > of the planner. Sometimes tables (ex: tables after having been first > propagated) need an "analyze" run on them, but it's not clear from an > "explain" output that the analyze statistics are faulty. Not even a > hint. > You can get a hint, sometimes, by comparing the predicted rows and the actual rows of an "explain (analyze)". Making this more user friendly to do would probably be best done by making an expert-system tool to look at the currently-reported plans (https://explain.depesz.com kind of does this already, particularly the row_x column) rather than trying to build something into core. It could be improved by detecting when the node being misestimated is simple scan of a single table or index with a single filter/condition, rather than a join or a complex filter/condition. > > So this is a feature request for an "EXPLAIN DETAILS" option or > something, basically like today's explain but with more "rationale" > included. This could be immensely useful to many Postgres users. > Unfortunately it would also be immensely hard to implement. What I would find useful is simply to have it report details about how the cost of each node of the plan was arrived at, i.e. how many of multiples of each of the *_cost factors were summed to arrive at the total cost. that would help me a lot in interpreting plans, and might also help someone like Depesz a lot in improving his expert-system. It would be far easier than what you are proposing, but would be still be a lot of work. It would probably also be challenged because the accounting overhead, while small, would be incurred on every single planner run. Cheers, Jeff
Re: [HACKERS] Tuplesort merge pre-reading
On Sat, Sep 10, 2016 at 12:04 AM, Heikki Linnakangas wrote: >> Did I misunderstand something? I'm applying the first patch of Peter's >> series (cap number of tapes), then your first one (remove prefetch) >> and second one (use larger read buffers). > > > Yes. I have been testing without Peter's first patch, with just the two > patches I posted. But it should work together (and does, I just tested) with > that one as well. You're going to need to rebase, since the root displace patch is based on top of my patch 0002-*, not Heikki's alternative. But, that should be very straightforward. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_sequence catalog
On 2016-09-10 17:23:21 +0530, Amit Kapila wrote: > On Thu, Sep 1, 2016 at 12:00 AM, Andres Freund wrote: > > On 2016-08-31 14:23:41 -0400, Tom Lane wrote: > >> Andres Freund writes: > >> > On 2016-08-31 13:59:48 -0400, Tom Lane wrote: > >> >> You are ignoring the performance costs associated with eating 100x more > >> >> shared buffer space than necessary. > >> > >> > I doubt that's measurable in any real-world scenario. You seldomly have > >> > hundreds of thousands of sequences that you all select from at a high > >> > rate. > >> > >> If there are only a few sequences in the database, cross-sequence > >> contention is not going to be a big issue anyway. > > > > Isn't that *precisely* when it's going to matter? If you have 5 active > > tables & sequences where the latter previously used independent locks, > > and they'd now be contending on a single lock. > > > > I may be missing something here, but why would it contend on a lock, > as per locking scheme proposed by Alvaro, access to sequence object > will need a share lock on buffer page. To make checkpointing/bgwriter work correctly we need exclusive locks on pages while writing..., or some new lock level preventing the page from being written out, while "shared dirtying" locks are being held. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GiST penalty functions [PoC]
>Personally I wouldn't be very happy about an IEEE754 assumption. Ok, so let's avoid autoconf man. There is no real explanations of the ground for this assumption, just a reference to paper of David Goldberg (and there is no metion about IEEE754 is absoulte everywhere). BTW, may be we can ask David Goldberg how to hack that float properly? I do not see any math sense in this: pack_float(float actualValue, int realm) { ... int realmAjustment = *((int*)&actualValue)/4; float something = *((float*)&realmAdjustment) ... } No wonder it's not in libs. Nither I see a way to implement it with ldexp siblings. >I did go to the trouble of testing Postgres on a VAX and we fixed the few instances where it had such dependencies for a net gain. Greg, could you please point out those places to see how exaclty it should be #ifdef'd? Regrads, Andrey Borodin. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)
On Fri, Sep 9, 2016 at 5:22 PM, Claudio Freire wrote: > Since it is true that doing so would make it impossible to keep the > asserts about tupindex in tuplesort_heap_root_displace, I guess it > depends on how useful those asserts are (ie: how likely it is that > those conditions could be violated, and how damaging it could be if > they were). If it is decided the refactor is desirable, I'd suggest > making the common siftup producedure static inline, to allow > tuplesort_heap_root_displace to inline and specialize it, since it > will be called with checkIndex=False and that simplifies the resulting > code considerably. Right. I want to keep it as a separate function for all these reasons. I also think that I'll end up further optimizing what I've called tuplesort_heap_root_displace in the future, to adopt to clustered input. I'm thinking of something like Timsort's "galloping mode". What I've come up with here still needs 2 comparisons and a swap per call for presorted input. There is still a missed opportunity for clustered or (inverse) correlated input -- we can make merging opportunistically skip ahead to determine that the root tape's 100th tuple (say) would still fit in the root position of the merge minheap. So, immediately return 100 tuples from the root's tape without bothering to compare them to anything. Do a binary search to find the best candidate minheap root before the 100th tuple if a guess of 100 doesn't work out. Adapt to trends. Stuff like that. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Use LEFT JOINs in some system views in case referenced row doesn
I wrote: > Fujii Masao writes: >> On Sat, Aug 20, 2016 at 6:13 AM, Tom Lane wrote: >>> Use LEFT JOINs in some system views in case referenced row doesn't exist. >> This change causes pg_stat_activity to report the "bogus" information about >> walsenders as follows. > Hmm ... but if we want to exclude walsenders from that view, surely we > should do so explicitly, not have it happen as a hidden side-effect > of not being able to join them to pg_database. I looked into this a little. There are at least three things we could do here: 1. Decide that showing walsenders is a good thing. I'm not really sure why it isn't -- for example, we could take the trouble to display the current replication command as the walsender's activity. 2. Suppress walsenders by the expedient of ignoring PGPROCs that have zero database ID. This would also ignore other process types that aren't connected to a specific database. This is pretty backwards-compatible because it's equivalent to what used to happen implicitly via the inner join in the view. 3. Suppress walsenders by adding some new field to PgBackendStatus that identifies them, and having pg_stat_get_activity() use that to filter. It looks to me like even before the change Fujii-san complains of, it was only generic walsenders that were hidden in the view. Database-specific walsenders (am_db_walsender) would have a valid database OID in PgBackendStatus and therefore would pass the join. This makes me think that option #1 is really the sanest alternative. While option #2 is the most backwards-compatible, it's pretty hard to see why we would think that showing only database-specific walsenders is a desirable behavior. If we want to exclude walsenders altogether, we need option #3. Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless dependency assumption libxml2 -> libxslt in MSVC scripts
Michael Paquier writes: > Thanks. I was a bit too lazy to look at the history to get this piece > of history out... And so the code that is presently in the MSVC > scripts should have been updated at the same time as those > compilations have been relaxed, but it got forgotten on the way I > guess. Then it seemt to me that the attached patch would do things as > they should be done: removal of the condition for iconv, which is an > optional flag for libxml2, and reversing the check for libxslt <-> > libxml2. Actually, wait a minute. Doesn't this bit in Mkvcbuild.pm need adjusted? if ($solution->{options}->{xml}) { $contrib_extraincludes->{'pgxml'} = [ $solution->{options}->{xml} . '/include', $solution->{options}->{xslt} . '/include', $solution->{options}->{iconv} . '/include' ]; $contrib_extralibs->{'pgxml'} = [ $solution->{options}->{xml} . '/lib/libxml2.lib', $solution->{options}->{xslt} . '/lib/libxslt.lib' ]; } It might accidentally fail to fail as-is, but likely it would be better not to be pushing garbage paths into extraincludes and extralibs when some of those options aren't set. 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] Useless dependency assumption libxml2 -> libxslt in MSVC scripts
Michael Paquier writes: > Thanks. I was a bit too lazy to look at the history to get this piece > of history out... And so the code that is presently in the MSVC > scripts should have been updated at the same time as those > compilations have been relaxed, but it got forgotten on the way I > guess. Then it seemt to me that the attached patch would do things as > they should be done: removal of the condition for iconv, which is an > optional flag for libxml2, and reversing the check for libxslt <-> > libxml2. Looks sane to me, will push. 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] COPY command with RLS bug
> Looking for and improving test coverage for RLS is a good suggestion, > but let's not link the fate of the issue reported here with this > requirement. I have spent some time looking at this patch and this > looks in rather good shape to me (you even remembered to use the > prefix regress_* for the role name that you are adding!). So I have > marked this bug fix as ready for committer. Excellent, thanks for the review and feedback. I appreciate it. -Adam -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Allow to_date() and to_timestamp() to accept localized month names
Mattia writes: > attached is a patch which adds support to localized month names in > to_date() and to_timestamp() functions. Seems like a fine goal. > I thought about reusing from_char_seq_search() but localized month > names use different capitalization according to the language grammar, > so I used pg_strncasecmp to do the match. pg_str(n)casecmp is really only meant to handle comparisons of ASCII strings; it will definitely not succeed in case-folding multibyte characters. That's not a big problem for to_date's existing usages but I'm afraid it will be for non-English month names. I think you'll need another solution there. You might have to resort to what citext does, namely apply the full lower() transformation, at least whenever the data string actually contains MB characters. > Regression tests with TM modifier are difficult since one should have > the locale used for the test installed on his system. I suspect you'll have to give up on putting much about this into the standard regression tests. We've used not-run-by-default test scripts in some similar cases (eg collate.linux.utf8.sql), but personally I think those are 99% a waste of time, precisely because they never actually get run by anyone but the author. 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] WAL consistency check facility
On Sat, Sep 10, 2016 at 3:19 AM, Michael Paquier wrote: > On Fri, Sep 9, 2016 at 4:01 PM, Kuntal Ghosh > wrote: - If WAL consistency check is enabled for a rmgrID, we always include the backup image in the WAL record. >>> >>> What happens if wal_consistency has different settings on a standby >>> and its master? If for example it is set to 'all' on the standby, and >>> 'none' on the master, or vice-versa, how do things react? An update of >>> this parameter should be WAL-logged, no? >>> >> It is possible to set wal_consistency to 'All' in master and any other >> values in standby. But, the scenario you mentioned will cause error in >> standby since it may not get the required backup image for wal >> consistency check. I think that user should be responsible to set >> this value correctly. We can improve the error message to make the >> user aware of the situation. > > Let's be careful here. You should as well consider things from the > angle that some parameter updates are WAL-logged as well, like > wal_level with the WAL record XLOG_PARAMETER_CHANGE. It seems entirely unnecessary for the master and the standby to agree here. I think what we need is two GUCs. One of them, which affects only the master, controls whether the validation information is including in the WAL, and the other, which affects only the standby, affects whether validation is performed when the necessary information is present. Or maybe skip the second one and just decree that standbys will always validate if the necessary information is present. Using the same GUC on both the master and the standby but making it mean different things in each of those places (whether to log the validation info in one case, whether to perform validation in the other case) is another option that also avoids needing to enforce that the setting is the same in both places, but probably an inferior one. -- 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] An extra error for client disconnection on Windows
Michael Paquier writes: > On Fri, Sep 9, 2016 at 11:39 PM, Tom Lane wrote: >> So this change would deal nicely with the "peer application on the remote >> host is suddenly stopped" case, at the price of being not nice about any >> of the other cases. Not convinced it's a good tradeoff. > Yes, in the list of failure cases that could trigger this error, the > one that looks like a problem is to me is when a network interface is > disabled. It may be a good idea to let users know via the logs that > something was connected. Could we for example log a WARNING message, > and not report an error? It isn't an "error". These conditions get logged at COMMERROR which is effectively LOG_SERVER_ONLY. 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] pg_sequence catalog
On Thu, Sep 1, 2016 at 12:00 AM, Andres Freund wrote: > On 2016-08-31 14:23:41 -0400, Tom Lane wrote: >> Andres Freund writes: >> > On 2016-08-31 13:59:48 -0400, Tom Lane wrote: >> >> You are ignoring the performance costs associated with eating 100x more >> >> shared buffer space than necessary. >> >> > I doubt that's measurable in any real-world scenario. You seldomly have >> > hundreds of thousands of sequences that you all select from at a high >> > rate. >> >> If there are only a few sequences in the database, cross-sequence >> contention is not going to be a big issue anyway. > > Isn't that *precisely* when it's going to matter? If you have 5 active > tables & sequences where the latter previously used independent locks, > and they'd now be contending on a single lock. > I may be missing something here, but why would it contend on a lock, as per locking scheme proposed by Alvaro, access to sequence object will need a share lock on buffer page. -- 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] Re: [COMMITTERS] pgsql: Change the way that LWLocks for extensions are allocated.
On Tue, Aug 30, 2016 at 8:01 AM, Andres Freund wrote: > On 2016-08-30 07:57:19 +0530, Amit Kapila wrote: >> I will write such a test case either in this week or early next week. > > Great. > Okay, attached patch adds some simple tests for pg_stat_statements. One thing to note is that we can't add pg_stat_statements tests for installcheck as this module requires shared_preload_libraries to be set. Hope this suffices the need. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com test_pg_stat_statements-v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] improved DefElem list processing
On 8/22/16 10:28 AM, Alvaro Herrera wrote: > Peter Eisentraut wrote: > >> I'm not happy that utils/acl.h has prototypes for aclchk.c, because >> acl.h is included all over the place. Perhaps I should make a >> src/include/catalog/aclchk.c to clean that up. > > I've been bothered by that too in the past. +1 for the cleanup. Here is a patch for that. It didn't quite achieve the elegance I was hoping for. Most uses of acl.h actually use aclchk.c functions, and the new aclchk.h must include acl.h, so basically you end up just changing most includes of acl.h to aclchk.h while still effectively including acl.h everywhere. But I think having one header file serve two separate .c files is still a confusing pattern that is worth cleaning up. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index d4f9090..06041f0 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -38,6 +38,7 @@ #include "access/htup_details.h" #include "access/reloptions.h" +#include "catalog/aclchk.h" #include "catalog/indexing.h" #include "catalog/namespace.h" #include "catalog/pg_foreign_server.h" @@ -50,7 +51,6 @@ #include "mb/pg_wchar.h" #include "miscadmin.h" #include "parser/scansup.h" -#include "utils/acl.h" #include "utils/builtins.h" #include "utils/fmgroids.h" #include "utils/guc.h" diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c index c3a518c..d312d74 100644 --- a/contrib/pg_prewarm/pg_prewarm.c +++ b/contrib/pg_prewarm/pg_prewarm.c @@ -16,12 +16,12 @@ #include #include "access/heapam.h" +#include "catalog/aclchk.h" #include "catalog/catalog.h" #include "fmgr.h" #include "miscadmin.h" #include "storage/bufmgr.h" #include "storage/smgr.h" -#include "utils/acl.h" #include "utils/builtins.h" #include "utils/lsyscache.h" #include "utils/rel.h" diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c index e20e7f8..1c8fff0 100644 --- a/contrib/pgrowlocks/pgrowlocks.c +++ b/contrib/pgrowlocks/pgrowlocks.c @@ -27,12 +27,12 @@ #include "access/multixact.h" #include "access/relscan.h" #include "access/xact.h" +#include "catalog/aclchk.h" #include "catalog/namespace.h" #include "funcapi.h" #include "miscadmin.h" #include "storage/bufmgr.h" #include "storage/procarray.h" -#include "utils/acl.h" #include "utils/builtins.h" #include "utils/rel.h" #include "utils/snapmgr.h" diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 1b45a4c..4a48e92 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -22,6 +22,7 @@ #include "access/reloptions.h" #include "access/relscan.h" #include "access/xloginsert.h" +#include "catalog/aclchk.h" #include "catalog/index.h" #include "catalog/pg_am.h" #include "miscadmin.h" diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c index b56d0e3..a1b213e 100644 --- a/src/backend/access/common/tupdesc.c +++ b/src/backend/access/common/tupdesc.c @@ -20,10 +20,10 @@ #include "postgres.h" #include "access/htup_details.h" +#include "catalog/aclchk.h" #include "catalog/pg_type.h" #include "miscadmin.h" #include "parser/parse_type.h" -#include "utils/acl.h" #include "utils/builtins.h" #include "utils/resowner_private.h" #include "utils/syscache.h" diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c index 6b709db..2305782 100644 --- a/src/backend/access/gin/ginfast.c +++ b/src/backend/access/gin/ginfast.c @@ -22,11 +22,11 @@ #include "access/xloginsert.h" #include "access/xlog.h" #include "commands/vacuum.h" +#include "catalog/aclchk.h" #include "catalog/pg_am.h" #include "miscadmin.h" #include "utils/memutils.h" #include "utils/rel.h" -#include "utils/acl.h" #include "postmaster/autovacuum.h" #include "storage/indexfsm.h" #include "storage/lmgr.h" diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c index 65c941d..c3a1997 100644 --- a/src/backend/access/index/genam.c +++ b/src/backend/access/index/genam.c @@ -21,11 +21,11 @@ #include "access/relscan.h" #include "access/transam.h" +#include "catalog/aclchk.h" #include "catalog/index.h" #include "lib/stringinfo.h" #include "miscadmin.h" #include "storage/bufmgr.h" -#include "utils/acl.h" #include "utils/builtins.h" #include "utils/lsyscache.h" #include "utils/rel.h" diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index a585c3a..9face59 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -22,6 +22,7 @@ #include "access/htup_details.h" #include "access/sysattr.h" #include "access/xact.h" +#include "catalog/aclchk.h" #include "catalog/binary_upgrade.h" #include "catalog/catalog.h" #include "catalog/dependency.h" @@ -59,7 +60,6 @@ #include "nodes/makefuncs.h" #include "parser/parse_func.h
Re: [HACKERS] pg_sequence catalog
On 9/5/16 10:35 PM, Tom Lane wrote: > In this viewpoint, we'd keep the sequence-specific data in a pg_sequence > catalog. pg_sequence rows would be extensions of the associated pg_class > rows in much the same way that pg_index rows extend the pg_class entries > for indexes. We should supply a view pg_sequences that performs the > implied join, and encourage users to select from that rather than directly > from pg_sequence (compare pg_indexes view). Let's start with that. Here is a patch that adds a pg_sequences view in the style of pg_tables, pg_indexes, etc. This seems useful independent of anything else, but would give us more freedom to change things around behind the scenes. A slight naming wart: I added a function lastval(regclass) for internal use to get a sequence's "last value". But we already have a public function lastval(), which gets the most recent nextval() result of any sequence. Those are two quite different things. I don't want to abandon the term "last value" here, however, because that is what the sequence relation uses internally, and also Oracle uses it in its system views with the same semantics that I propose here. We could use a more verbose name like sequence_last_value(regclass), perhaps. lastval has been kept separate from pg_sequence_parameters, because if we were to go ahead with a new catalog layout later, then pg_sequence_parameters would become obsolescent while we would possibly still need a lastval function. The column names of the new view have been deliberately tuned to use a more conventional style than the information schema while avoiding what I would consider some past naming mistakes. (For example, I hate "is_cycled", which reads like "sequence has wrapped around at least once in the past"). Here are some similar views in other places: https://docs.oracle.com/cd/B28359_01/server.111/b28320/statviews_2053.htm https://www.ibm.com/support/knowledgecenter/en/SSEPGG_9.7.0/com.ibm.db2.luw.sql.ref.doc/doc/r0004203.html https://msdn.microsoft.com/en-us/library/ff877934.aspx -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services From f74e1cc1f6ee4a56abc9f46c413c0af5086e1e40 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Fri, 9 Sep 2016 12:00:00 -0400 Subject: [PATCH] Add pg_sequences view Like pg_tables, pg_views, and others, this view contains information about sequences in a way that is independent of the system catalog layout but more comprehensive than the information schema. --- doc/src/sgml/catalogs.sgml | 90 src/backend/catalog/system_views.sql | 16 ++ src/backend/commands/sequence.c | 45 ++-- src/include/catalog/pg_proc.h| 4 +- src/include/commands/sequence.h | 1 + src/test/regress/expected/rules.out | 14 + src/test/regress/expected/sequence.out | 16 ++ src/test/regress/expected/sequence_1.out | 16 ++ src/test/regress/sql/sequence.sql| 7 +++ 9 files changed, 205 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 322d8d6..1c440e3 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -7379,6 +7379,11 @@ System Views + pg_sequences + sequences + + + pg_settings parameter settings @@ -9114,6 +9119,91 @@ pg_seclabels Columns + + pg_sequences + + + pg_sequences + + + + The view pg_sequences provides access to + useful information about each sequence in the database. + + + + pg_sequences Columns + + + + + Name + Type + References + Description + + + + + schemaname + name + pg_namespace.nspname + Name of schema containing sequence + + + sequencename + name + pg_class.relname + Name of sequence + + + sequenceowner + name + pg_authid.rolname + Name of sequence's owner + + + start_value + bigint + + Start value of the sequence + + + min_value + bigint + + Minimum value of the sequence + + + max_value + bigint + + Maximum value of the sequence + + + increment_by + bigint + + Increment value of the sequence + + + cycle + boolean + + Whether the sequence cycles + + + cache_size + bigint + + Cache size of the sequence + + + + + + + pg_settings diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index ada2142..99a9b41 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -158,6 +158,22 @@ CREATE VIEW pg_indexes AS LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
Re: [HACKERS] Block level parallel vacuum WIP
On Wed, Aug 24, 2016 at 3:31 AM, Michael Paquier wrote: > On Tue, Aug 23, 2016 at 10:50 PM, Amit Kapila > wrote: > > On Tue, Aug 23, 2016 at 6:11 PM, Michael Paquier > > wrote: > >> On Tue, Aug 23, 2016 at 8:02 PM, Masahiko Sawada > wrote: > >>> As for PoC, I implemented parallel vacuum so that each worker > >>> processes both 1 and 2 phases for particular block range. > >>> Suppose we vacuum 1000 blocks table with 4 workers, each worker > >>> processes 250 consecutive blocks in phase 1 and then reclaims dead > >>> tuples from heap and indexes (phase 2). > >> > >> So each worker is assigned a range of blocks, and processes them in > >> parallel? This does not sound performance-wise. I recall Robert and > >> Amit emails on the matter for sequential scan that this would suck > >> performance out particularly for rotating disks. > >> > > > > The implementation in patch is same as we have initially thought for > > sequential scan, but turned out that it is not good way to do because > > it can lead to inappropriate balance of work among workers. Suppose > > one worker is able to finish it's work, it won't be able to do more. > > Ah, so it was the reason. Thanks for confirming my doubts on what is > proposed. > -- > I believe Sawada-san has got enough feedback on the design to work out the next steps. It seems natural that the vacuum workers are assigned a portion of the heap to scan and collect dead tuples (similar to what patch does) and the same workers to be responsible for the second phase of heap scan. But as far as index scans are concerned, I agree with Tom that the best strategy is to assign a different index to each worker process and let them vacuum indexes in parallel. That way the work for each worker process is clearly cut out and they don't contend for the same resources, which means the first patch to allow multiple backends to wait for a cleanup buffer is not required. Later we could extend it further such multiple workers can vacuum a single index by splitting the work on physical boundaries, but even that will ensure clear demarkation of work and hence no contention on index blocks. ISTM this will require further work and it probably makes sense to mark the patch as "Returned with feedback" for now. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] pg_basebackup, pg_receivexlog and data durability (was: silent data loss with ext4 / all current versions)
On 3 Sep. 2016 9:22 pm, "Michael Paquier" wrote: > > On Sat, Sep 3, 2016 at 12:42 AM, Magnus Hagander wrote: > > On Fri, Sep 2, 2016 at 8:50 AM, Michael Paquier < michael.paqu...@gmail.com> > > wrote: > >> On Fri, Sep 2, 2016 at 2:20 AM, Peter Eisentraut > >> wrote: > >> > On 5/13/16 2:39 AM, Michael Paquier wrote: > >> What do others think about that? I could implement that on top of 0002 > >> with some extra options. But to be honest that looks to be just some > >> extra sugar for what is basically a bug fix... And I am feeling that > >> providing such a switch to users would be a way for one to shoot > >> himself badly, particularly for pg_receivexlog where a crash can cause > >> segments to go missing. > >> > > > > Well, why do we provide a --nosync option for initdb? Wouldn't the argument > > basically be the same? > > Yes, the good-for-testing-but-not-production argument. We need it for tap tests. More and more will use pg_basebackup and it'll start hurting test speeds badly.
Re: [HACKERS] Logical Replication WIP
Review of 0004-Make-libpqwalreceiver-reentrant.patch: This looks like a good change. typo: _PG_walreceirver_conn_init For libpqrcv_create_slot(), slotname should be const char *. Similarly, for slotname in libpqrcv_startstreaming*() and conninfo in libpqrcv_connect(). (the latter two pre-existing) The connection handle should record in libpqrcv_connect() whether a connection is a logical or physical replication stream. Then that parameter doesn't have to be passed around later (or at least some asserts could double-check it). In libpqrcv_connect(), the new argument connname is actually just the application name, for which in later patches the subscription name is passed in. Does this have a deeper meaning, or should we call the argument appname to avoid introducing another term? New function libpqrcv_create_slot(): Hardcoded cmd length (hmm, other functions do that too), should used StringInfo. ereport instead of elog. No newline at the end of error message, since PQerrorMessage() already supplies it. Typo "could not crate". Briefly document return value. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] Allow to_date() and to_timestamp() to accept localized month names
Hi, attached is a patch which adds support to localized month names in to_date() and to_timestamp() functions. The patch is fairly simple but I want to discuss the approach and implementation: Using the TM modifier as in to_char() was already discussed some years ago: 10710.1202170...@sss.pgh.pa.us [1] I thought about reusing from_char_seq_search() but localized month names use different capitalization according to the language grammar, so I used pg_strncasecmp to do the match. Regression tests with TM modifier are difficult since one should have the locale used for the test installed on his system. Usage example: postgres=# set lc_time to 'fr_FR'; SET postgres=# select to_date('22 janvier 2016', 'DD TMMonth '); to_date 2016-01-22 (1 row) [1] https://www.postgresql.org/message-id/10710.1202170898%40sss.pgh.pa.us Thanks Mattia localized_month_names_v1.patch Description: application/download -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup, pg_receivexlog and data durability (was: silent data loss with ext4 / all current versions)
On 9/3/16 9:23 AM, Michael Paquier wrote: > On Sat, Sep 3, 2016 at 10:22 PM, Michael Paquier > wrote: >> On Sat, Sep 3, 2016 at 10:21 PM, Michael Paquier >> wrote: >>> Oh, well. I have just implemented it on top of the two other patches >>> for pg_basebackup. For pg_receivexlog, I am wondering if it makes >>> sense to have it. That would be trivial to implement it, and I think >>> that we had better make the combination of --synchronous and --nosync >>> just leave with an error. Thoughts about having that for >>> pg_receivexlog? >> >> With patches that's actually better.. > > Meh, meh et meh. In fsync_pgdata(), you have removed the progname from one error message, even though it is passed into the function. Also, I think fsync_pgdata() should not be printing initdb's progress messages. That should stay in initdb. Worse, in 0002 you are then changing the output, presumably to suit pg_basebackup or something else, which messes up the initdb output. durable_rename() does not fsync an existing newfile before renaming, in contrast to the backend code in fd.c. I'm slightly concerned about lots of code duplication in durable_rename, fsync_parent_path between backend and standalone code. Also, I'm equally slightly concerned that having duplicate function names will mess up tag search for someone. This is a preexisting mistake, but fsync_fname_ext() should just be fsync_fname() to match the backend naming. In the backend, fsync_fname_ext() "extends" fsync_fname() by accepting an ignore_perm argument, but the initdb code doesn't do that. I don't think tar file output in pg_basebackup needs to be fsynced. It's just a backup file like what pg_dump produces, and we don't fsync that either. The point of this change is to leave a data directory in a synced state equivalent to what initdb leaves behind. Some of the changes in receivelog.c seem excessive. Replacing a plain fsync() with fsync_fname_ext() plus fsync_parent_path() isn't justified by the references you have provided. This would need more explanation. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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: Exclude additional directories in pg_basebackup
On Fri, Sep 9, 2016 at 10:18 PM, David Steele wrote: > On 9/6/16 10:25 PM, Michael Paquier wrote: >> >> >> On Wed, Sep 7, 2016 at 12:16 AM, David Steele wrote: >> >>> Attached is a new patch that adds sgml documentation. I can expand on >>> each >>> directory individually if you think that's necessary, but thought it was >>> better to lump them into a few categories. >> >> >> +be ommitted from the backup as they will be initialized on postmaster >> +startup. If the is set and >> is >> +under the database cluster directory then the contents of the >> directory >> +specified by can also >> be ommitted. >> >> s/ommitted/omitted/ > > > Fixed. > >> +#define EXCLUDE_DIR_MAX 8 >> +#define EXCLUDE_DIR_STAT_TMP 0 >> + >> +const char *excludeDirContents[EXCLUDE_DIR_MAX] = >> +{ >> +/* >> + * Skip temporary statistics files. The first array position will be >> + * filled with the value of pgstat_stat_directory relative to PGDATA. >> + * PG_STAT_TMP_DIR must be skipped even when stats_temp_directory is >> set >> + * because PGSS_TEXT_FILE is always created there. >> + */ >> +NULL, >> I find that ugly. I'd rather use an array with undefined size for the >> fixed elements finishing by NULL, remove EXCLUDE_DIR_MAX and >> EXCLUDE_DIR_STAT_TMP and use a small routine to do the work done on >> _tarWriteHeader... > > > Done. Also writing out pg_xlog with the new routine. Thanks, this looks in far better shape now. + /* Removed on startup, see DeleteAllExportedSnapshotFiles(). */ + "pg_snapshots", Using SNAPSHOT_EXPORT_DIR is possible here. +_tarWriteDir(char *pathbuf, int basepathlen, struct stat *statbuf, +bool sizeonly) That's nice, thanks! This even takes care of the fact when directories other than pg_xlog are symlinks or junction points. + /* Recreated on startup, see StartupReplicationSlots(). */ + "pg_replslot", This comment is incorrect, pg_replslot is skipped in a base backup because that's just useless. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] An extra error for client disconnection on Windows
On Fri, Sep 9, 2016 at 11:39 PM, Tom Lane wrote: > Haribabu Kommi writes: >> On Thu, Jun 2, 2016 at 6:51 PM, Kyotaro HORIGUCHI < >> horiguchi.kyot...@lab.ntt.co.jp> wrote: >>> After a process termination without PQfinish() of a client, >>> server emits the following log message not seen on Linux boxes. >>> LOG: could not receive data from client: An existing connection was >>> forcibly closed by the remote host. >>> >>> This patch translates WSAECONNRESET of WSARecv to an EOF so that >>> pgwin32_recv behaves the same way with Linux. > >> Marked the patch as "ready for committer". > > Windows is not my platform, but ... is this actually an improvement? > I'm fairly concerned that this change would mask real errors that ought > to get logged. I don't know that that's an okay price to pay for > suppressing a log message when clients violate the protocol. > > According to > https://msdn.microsoft.com/en-us/library/windows/desktop/ms740668(v=vs.85).aspx > WSAECONNRESET means: > > An existing connection was forcibly closed by the remote host. This > normally results if the peer application on the remote host is > suddenly stopped, the host is rebooted, the host or remote network > interface is disabled, or the remote host uses a hard close (see > setsockopt for more information on the SO_LINGER option on the remote > socket). This error may also result if a connection was broken due to > keep-alive activity detecting a failure while one or more operations > are in progress. Operations that were in progress fail with > WSAENETRESET. Subsequent operations fail with WSAECONNRESET. > > (The description of WSAENETRESET, on the same page, indicates that the > last two sentences apply only to the keep-alive failure case.) > > So this change would deal nicely with the "peer application on the remote > host is suddenly stopped" case, at the price of being not nice about any > of the other cases. Not convinced it's a good tradeoff. Yes, in the list of failure cases that could trigger this error, the one that looks like a problem is to me is when a network interface is disabled. It may be a good idea to let users know via the logs that something was connected. Could we for example log a WARNING message, and not report an error? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WAL consistency check facility
On Fri, Sep 9, 2016 at 4:01 PM, Kuntal Ghosh wrote: >>> - If WAL consistency check is enabled for a rmgrID, we always include >>> the backup image in the WAL record. >> >> What happens if wal_consistency has different settings on a standby >> and its master? If for example it is set to 'all' on the standby, and >> 'none' on the master, or vice-versa, how do things react? An update of >> this parameter should be WAL-logged, no? >> > It is possible to set wal_consistency to 'All' in master and any other > values in standby. But, the scenario you mentioned will cause error in > standby since it may not get the required backup image for wal > consistency check. I think that user should be responsible to set > this value correctly. We can improve the error message to make the > user aware of the situation. Let's be careful here. You should as well consider things from the angle that some parameter updates are WAL-logged as well, like wal_level with the WAL record XLOG_PARAMETER_CHANGE. >>> - In recovery tests (src/test/recovery/t), I've added wal_consistency >>> parameter in the existing scripts. This feature doesn't change the >>> expected output. If there is any inconsistency, it can be verified in >>> corresponding log file. >> >> I am afraid that just generating a WARNING message is going to be >> useless for the buildfarm. If we want to detect errors, we could for >> example have an additional GUC to trigger an ERROR or a FATAL, taking >> down the cluster, and allowing things to show in red on a platform. >> > Yes, we can include an additional GUC to trigger an ERROR for any > inconsistency. I'd like to hear extra opinions about that, but IMO just having an ERROR would be fine for the first implementation. Once you've bumped into an ERROR, you are likely going to fix it first. >> + /* >> +* Followings are the rmids which can have backup blocks. >> +* We'll enable this feature only for these rmids. >> +*/ >> + newwalconsistency[RM_HEAP2_ID] = true; >> + newwalconsistency[RM_HEAP_ID] = true; >> + newwalconsistency[RM_BTREE_ID] = true; >> + newwalconsistency[RM_HASH_ID] = true; >> + newwalconsistency[RM_GIN_ID] = true; >> + newwalconsistency[RM_GIST_ID] = true; >> + newwalconsistency[RM_SEQ_ID] = true; >> + newwalconsistency[RM_SPGIST_ID] = true; >> + newwalconsistency[RM_BRIN_ID] = true; >> + newwalconsistency[RM_GENERIC_ID] = true; >> + newwalconsistency[RM_XLOG_ID] = true; >> Here you can just use MemSet with RM_MAX_ID and simplify this code >> maintenance. >> > Not all rmids can have backup blocks. So, for wal_consistency = 'all', > I've enabled only those rmids which can have backup blocks. Even if some rmgrs do not support FPWs, I don't think that it is safe to assume that the existing ones would never support it. Imagine for example that feature X is implemented. Feature X adds rmgs Y, but rmgr Y does not use FPWs. At a later point a new feature is added, which makes rmgr Y using FPWs. We'd increase the number of places to update with your patch, increasing the likelyness to introduce bugs. It would be better to use a safe implementation from the maintenance point of view to be honest (maintenance load of masking functions is somewhat leveraged by the fact that on-disk format is kept compatible). >> >> + if (pg_strcasecmp(tok, "Heap2") == 0) >> + { >> + newwalconsistency[RM_HEAP2_ID] = true; >> + } >> Thinking more about it, I guess that we had better change the >> definition list of rmgrs in rmgr.h and get something closer to >> RmgrDescData that pg_xlogdump has to avoid all this stanza by >> completing it with the name of the rmgr. The only special cases that >> this code path would need to take care of would be then 'none' and >> 'all'. You could do this refactoring on top of the main patch to >> simplify it as it is rather big (1.7k lines). >> > I'm not sure about this point. wal_consistency doesn't support all > the rmids. We should have some way to check this. I'd rather see this code done in such a way that all the rmgrs can be handled, this approach being particularly attractive for the fact that there is no need to change it if new rmgrs are added in the future. (This was a reason as well why I still think that a simple on/off switch would be plain enough, users have mostly control of the SQLs triggering WAL. And if you run tests, you'll likely have the mind to turn autovacuum to off to avoid it to generate FPWs and pollute the logs at least at the second run of your tests). And if you move forward with the approach of making this parameter a list, I think that it would be better to add a section in the WAL documentation about resource managers, like what they are, and list them in this section of the docs. Then your parameter could link to this documentation part and users would be able to see what k
Re: [HACKERS] Tuplesort merge pre-reading
On 09/10/2016 04:21 AM, Claudio Freire wrote: On Fri, Sep 9, 2016 at 9:51 PM, Claudio Freire wrote: On Fri, Sep 9, 2016 at 8:13 AM, Heikki Linnakangas wrote: Claudio, if you could also repeat the tests you ran on Peter's patch set on the other thread, with these patches, that'd be nice. These patches are effectively a replacement for 0002-Use-tuplesort-batch-memory-for-randomAccess-sorts.patch. And review would be much appreciated too, of course. Attached are new versions. Compared to last set, they contain a few comment fixes, and a change to the 2nd patch to not allocate tape buffers for tapes that were completely unused. Will do so Thanks! It seems both 1 and 1+2 break make check. Oh. Works for me. What's the failure you're getting? Did I misunderstand something? I'm applying the first patch of Peter's series (cap number of tapes), then your first one (remove prefetch) and second one (use larger read buffers). Yes. I have been testing without Peter's first patch, with just the two patches I posted. But it should work together (and does, I just tested) with that one as well. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Useless dependency assumption libxml2 -> libxslt in MSVC scripts
On Thu, Sep 8, 2016 at 10:04 PM, Tom Lane wrote: > The core code has never used xslt at all. Yes. > Some quick digging in the git > history suggests that contrib/xml2 wasn't very clean about this before > 2008: > [...] > Both of those fixes postdate our native-Windows port, though I'm not sure > of the origin of the specific test you're wondering about. Thanks. I was a bit too lazy to look at the history to get this piece of history out... And so the code that is presently in the MSVC scripts should have been updated at the same time as those compilations have been relaxed, but it got forgotten on the way I guess. Then it seemt to me that the attached patch would do things as they should be done: removal of the condition for iconv, which is an optional flag for libxml2, and reversing the check for libxslt <-> libxml2. -- Michael msvc_xml_relax_v2.patch Description: application/download -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers