Re: [HACKERS] less log level for success dynamic background workers for 9.5
2015-06-26 17:28 GMT+02:00 Robert Haas robertmh...@gmail.com: On Wed, Jun 24, 2015 at 7:39 PM, Jim Nasby jim.na...@bluetreble.com wrote: I think it's a whole separate topicto Pavel's original proposal though, and really merits a separate thread. For Pavel's issue I'm all in favour of just changing the log message, I think LOG is way too high for something that's internal implementation detail. +1. OK, I have committed Pavel's patch. Thank you Pavel -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] Schedule for 9.5alpha1
On Thu, Jun 25, 2015 at 11:55 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jun 25, 2015 at 6:25 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote: I have a serious open item reported 1.5 month ago then reminded several times has not been fixed up yet. 9a28c8860f777e439aa12e8aea7694f8010f3...@bpxm15gp.gisp.nec.co.jp Patch is less than 100 lines, entirely designed according to Tom's suggestion. The problem is, commit 1a8a4e5cde2b7755e11bde2ea7897bd650622d3e reverted create_plan_recurse() to static function, thus, extension lost way to transform Path node to Plan node if it wants to takes underlying child nodes, like SeqScan, HashJoin and so on. Tom's suggestion is to add a list of Path nodes on CustomPath structure, to be transformed by createplan.c, instead of public create_plan_recurse(). It is nearly obvious problem, and bugfix patch already exists. Yes, I am quite unhappy with this situation. Tom promised me at PGCon that he would look at this soon, but there is no sign that he has, and he said the same thing weeks ago. I think it can't be right to let this sit for another month or three. Even if the API you've implemented is worse than something Tom can design, it is certainly better than the status quo. I would rather have a working but imperfect API and have to break compatibility later in beta than have a non-working API. ...given which, I have committed this. I did not like the use of custom_children as a generic monicker, so I changed it to custom_paths, custom_plans, or custom_ps, as appropriate in each case. I did a bit of cosmetic cleanup as well. This does seem much nicer than giving custom plans direct access to create_plan_recurse(). The fact that you found various other places of using those lists demonstrates that nicely. Thanks for your help! One advantage of the approach, I think, is custom_paths list allows to implement N-way (N 2) join more naturally than just direct accesses to create_plan_recurse(). The example below shows contents of t1, t2 and t3 are enough small to load GPU RAM, so the custom GpuJoin can run these 4 tables join within a single step. postgres=# explain select * from t0 natural join t1 natural join t2 natural join t3; QUERY PLAN Custom Scan (GpuJoin) (cost=6501.77..249476.48 rows=9899296 width=176) Bulkload: On (density: 100.00%) Depth 1: Logic: GpuHashJoin, HashKeys: (cid), JoinQual: (cid = cid), nrows_ratio: 0.98995197 Depth 2: Logic: GpuHashJoin, HashKeys: (aid), JoinQual: (aid = aid), nrows_ratio: 1. Depth 3: Logic: GpuHashJoin, HashKeys: (bid), JoinQual: (bid = bid), nrows_ratio: 1. - Custom Scan (BulkScan) on t0 (cost=0.00..242855.74 rows=774 width=77) - Seq Scan on t3 (cost=0.00..734.00 rows=4 width=37) - Seq Scan on t1 (cost=0.00..734.00 rows=4 width=37) - Seq Scan on t2 (cost=0.00..734.00 rows=4 width=37) (9 rows) Best regards, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
Fujita-san, BTW, if you try newer version of postgres_fdw foreign join patch, please provide me to reproduce the problem/ OK Did you forget to attach the patch, or v17 is in use? I'd like to suggest a solution that re-construct remote tuple according to the fdw_scan_tlist on ExecScanFetch, if given scanrelid == 0. It enables to run local qualifier associated with the ForeignScan node, and it will also work for the case when tuple in es_epqTupleSet[] was local heap. Maybe I'm missing something, but I don't think your proposal works properly because we don't have any component ForeignScan state node or subsidiary join state node once we've replaced the entire join with the ForeignScan performing the join remotely, IIUC. So, my image was to have another subplan for EvalPlanQual as well as the ForeignScan, to do the entire join locally for the component test tuples if we are inside an EvalPlanQual recheck. Hmm... Probably, we have two standpoints to tackle the problem. The first standpoint tries to handle the base foreign table as a prime relation for locking. Thus, we have to provide a way to fetch a remote tuple identified with the supplied ctid. The advantage of this approach is the way to fetch tuples from base relation is quite similar to the existing form, however, its disadvantage is another side of the same coin, because the ForeignScan node with scanrelid==0 (that represents remote join query) may have local qualifiers which shall run on the tuple according to fdw_scan_tlist. One other standpoint tries to handle a bunch of base foreign tables as a unit. That means, if any of base foreign table is the target of locking, it prompts FDW driver to fetch the latest joined tuple identified by ctid, even if this join contains multiple base relations to be locked. The advantage of this approach is that we can use qualifiers of the ForeignScan node with scanrelid==0 and no need to pay attention of remote qualifier and/or join condition individually. Its disadvantage is, we may extend EState structure to keep the joined tuples, in addition to es_epqTupleSet[]. I'm inclined to think the later standpoint works well, because it does not need to reproduce an alternative execution path in local side again, even if a ForeignScan node represents much complicated remote query. If we would fetch tuples of individual base relations, we need to reconstruct identical join path to be executed on remote- side, don't it? IIUC, the purpose of EvalPlanQual() is to ensure the tuples to be locked is still visible, so it is not an essential condition to fetch base tuples individually. Just an aside, please tell me if someone know, does EvalPlanQual logic work correctly even if the tuple to be locked located in the right tree of HashJoin? In this case, it seems to me ExecHashJoin does not refresh Hash table again even if ExecProcNode() is invoked with es_epqTupleSet[], thus, old tuple is already visible and checked, isn't it? Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei kai...@ak.jp.nec.com -Original Message- From: pgsql-hackers-ow...@postgresql.org [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Etsuro Fujita Sent: Thursday, June 25, 2015 3:12 PM To: Kaigai Kouhei(海外 浩平) Cc: PostgreSQL-development Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual Hi KaiGai-san, I'd like to work on this issue with you! On 2015/06/25 10:48, Kouhei Kaigai wrote: I'd like to suggest a solution that re-construct remote tuple according to the fdw_scan_tlist on ExecScanFetch, if given scanrelid == 0. It enables to run local qualifier associated with the ForeignScan node, and it will also work for the case when tuple in es_epqTupleSet[] was local heap. Maybe I'm missing something, but I don't think your proposal works properly because we don't have any component ForeignScan state node or subsidiary join state node once we've replaced the entire join with the ForeignScan performing the join remotely, IIUC. So, my image was to have another subplan for EvalPlanQual as well as the ForeignScan, to do the entire join locally for the component test tuples if we are inside an EvalPlanQual recheck. BTW, if you try newer version of postgres_fdw foreign join patch, please provide me to reproduce the problem/ OK Also, as an aside, postgres_fdw does not implement RefetchForeignRow() at this moment. Does it make a problem? I don't think so, though I think it would be better to test that the foreign join pushdown API patch also allows late row locking using the postgres_fdw. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription:
Re: [HACKERS] drop/truncate table sucks for large values of shared buffers
Amit Kapila amit.kapil...@gmail.com writes: I have looked into it and found that the main reason for such a behaviour is that for those operations it traverses whole shared_buffers and it seems to me that we don't need that especially for not-so-big tables. We can optimize that path by looking into buff mapping table for the pages that exist in shared_buffers for the case when table size is less than some threshold (say 25%) of shared buffers. I don't like this too much because it will fail badly if the caller is wrong about the maximum possible page number for the table, which seems not exactly far-fetched. (For instance, remember those kernel bugs we've seen that cause lseek to lie about the EOF position?) It also offers no hope of a fix for the other operations that scan the whole buffer pool, such as DROP TABLESPACE and DROP DATABASE. In the past we've speculated about fixing the performance of these things by complicating the buffer lookup mechanism enough so that it could do find any page for this table/tablespace/database efficiently. Nobody's had ideas that seemed sane performance-wise though. 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] Semantics of pg_file_settings view
Amit Kapila amit.kapil...@gmail.com writes: On Fri, Jun 26, 2015 at 9:47 PM, Tom Lane t...@sss.pgh.pa.us wrote: What we evidently need to do is fix things so that the pg_file_settings data gets captured before we suppress duplicates. The simplest change would be to move the whole thing to around line 208 in guc-file.l, just after the stanza that loads PG_AUTOCONF_FILENAME. Or you could argue that the approach is broken altogether, and that we should capture the data while we read the files, so that you have some useful data in the view even if ParseConfigFile later decides there's a syntax error. I'm actually thinking maybe we should flush that data-capturing logic altogether in favor of just not deleting the ConfigVariable list data structure, and generating the view directly from that data structure. Idea for generating view form ConfigVariable list sounds good, but how will it preserve the duplicate entries in the list assuming either we need to revert the original fix (e3da0d4d1) or doing the same in loop where we set GUC_IS_IN_FILE? I'm thinking of adding an ignore boolean flag to ConfigVariable, which the GUC_IS_IN_FILE loop would set in ConfigVariables that are superseded by later list entries. Then the GUC-application loop would just skip those entries. This would be good because the flag could be displayed somehow in the pg_file_settings view, whereas right now you have to manually check for duplicates. Keeping removal of duplicate items in ParseConfigFp() has the advantage that it will work for all other places from where ParseConfigFp() is called, though I am not sure if today that is required. I don't think it is; if it were, we'd have had other complaints about that, considering that 9.4.0 is the *only* release we've ever shipped that suppressed duplicates right inside ParseConfigFp(). I would in fact turn that argument on its head, and state that Fujii-san's patch was probably ill-conceived because it implicitly assumes that duplicate suppression is okay for every caller of ParseConfigFp. It's not hard to imagine use-cases that that would break, even if we have none today. 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
[HACKERS] Bogus postmaster-only contexts laying about in backends
I happened to notice these in a backend's memory context dump: ident parser context: 0 total in 0 blocks; 0 free (0 chunks); 0 used hba parser context: 7168 total in 3 blocks; 3920 free (1 chunks); 3248 used Is there a good reason why these weren't made children of the PostmasterContext, so that they'd go away inside backends? 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] drop/truncate table sucks for large values of shared buffers
On Fri, Jun 26, 2015 at 9:45 PM, Amit Kapila amit.kapil...@gmail.com wrote: Sometime back on one of the PostgreSQL blog [1], there was discussion about the performance of drop/truncate table for large values of shared_buffers and it seems that as the value of shared_buffers increase the performance of drop/truncate table becomes worse. I think those are not often used operations, so it never became priority to look into improving them if possible. I have looked into it and found that the main reason for such a behaviour is that for those operations it traverses whole shared_buffers and it seems to me that we don't need that especially for not-so-big tables. We can optimize that path by looking into buff mapping table for the pages that exist in shared_buffers for the case when table size is less than some threshold (say 25%) of shared buffers. Attached patch implements the above idea and I found that performance doesn't dip much with patch even with large value of shared_buffers. I have also attached script and sql file used to take performance data. +1 for the effort to improve this. With your technique added, there are 3 possible ways the search can happen a) Scan NBuffers and scan list of relations, b) Scan NBuffers and bsearch list of relations, and c) Scan list of relations and then invalidate blocks of each fork from shared buffers. Would it be worth it finding one technique that can serve decently from the low-end shared_buffers to the high-end. On patch: There are multiple naming styles being used in DropForkSpecificBuffers(); my_name and myName. Given this is a new function, it'd help to be consistent. s/blk_count/blockNum/ s/new//, for eg. newTag, because there's no corresponding tag/oldTag variable in the function. s/blocksToDel/blocksToDrop/. BTW, we never pass anything other than the total number of blocks in the fork, so we may as well call it just numBlocks. s/traverse_buf_freelist/scan_shared_buffers/, because when it is true, we scan the whole shared_buffers. s/rel_count/rel_num/ Reduce indentation/tab in header-comments of DropForkSpecificBuffers(). But I see there's precedent in neighboring functions, so this may be okay. Doing pfree() of num_blocks, num_fsm_blocks and num_vm_blocks in one place (instead of two, at different indentation levels) would help readability. Best regards, -- Gurjeet Singh http://gurjeet.singh.im/
Re: [HACKERS] Rework the way multixact truncations work
On 2015-06-26 14:48:35 -0300, Alvaro Herrera wrote: Andres Freund wrote: Rework the way multixact truncations work. I spent some time this morning reviewing this patch and had some comments that I relayed over IM to Andres. Thanks for that! 2. We set PGXACT-delayChkpt while the truncation is executed. This seems reasonable, and there's a good reason for it, but all the other users of this facility only do small operations with this thing grabbed, while the multixact truncation could take a long time because a large number of files might be deleted. Maybe it's not a problem to have checkpoints be delayed by several seconds, or who knows maybe even a minute in a busy system. (We will have checkpointer sleeping in 10ms intervals until the truncation is complete). I don't think this is a problem. Consider that we're doing all this in the checkpointer today, blocking much more than just the actual xlog insertion. That's a bigger problem, as we'll not do the paced writing during that and such. The worst thatthis can cause is a bunch of sleeps, that seems fairly harmless. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Removing SSL renegotiation (Was: Should we back-patch SSL renegotiation fixes?)
On 2015-06-27 15:07:05 +0900, Michael Paquier wrote: On Sat, Jun 27, 2015 at 6:12 AM, Tom Lane t...@sss.pgh.pa.us wrote: Andres Freund and...@anarazel.de writes: On 2015-06-24 16:41:48 +0200, Andres Freund wrote: I, by now, have come to a different conclusion. I think it's time to entirely drop the renegotiation support. I think by now we essentially concluded that we should do that. What I'm not sure yet is how: Do we want to rip it out in master and just change the default in the backbranches, or do we want to rip it out in all branches and leave a faux guc in place in the back branches. I vote for the latter, but would be ok with both variants. I think the former is probably the saner answer. It is less likely to annoy people who dislike back-branch changes. And it will be significantly less work, considering that that code has changed enough that you won't be able to just cherry-pick a removal patch. I also fear there's a nonzero chance of breaking stuff if you're careless about doing the removal in one or more of the five active back branches ... +1 for removing on master and just disabling on back-branches. The problem with that approach is that it leaves people hanging in the dry if they've uncommented the default value, or changed it. That doesn't seem nice to me. 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] Removing SSL renegotiation (Was: Should we back-patch SSL renegotiation fixes?)
Andres Freund and...@anarazel.de writes: On 2015-06-27 15:07:05 +0900, Michael Paquier wrote: +1 for removing on master and just disabling on back-branches. The problem with that approach is that it leaves people hanging in the dry if they've uncommented the default value, or changed it. That doesn't seem nice to me. I think at least 99% of the people who are using a nondefault value of ssl_renegotiation_limit are using zero and so would have no problem with this at all. Possibly 100% of them; there's not really much use-case for changing from 512MB to some other nonzero value, is there? 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
[HACKERS] pg_file_settings view vs. Windows
I noticed that in EXEC_BACKEND builds (ie, Windows) the pg_file_settings view doesn't act as its author presumably intended. Specifically, it reads as empty until/unless the current session processes a SIGHUP event. This is because before that happens, it's depending on having inherited the state data from the postmaster via fork(), which of course does not happen with EXEC_BACKEND. This applies to both the committed version and my proposed rewrite. AFAICS the only correct fix would be to add the pg_file_settings state data to the set of data dumped and reloaded through write_nondefault_variables/read_nondefault_variables. That would add a fair amount of code, and it might hurt backend startup time more than the feature is worth. In any case, I'm not volunteering to do it. More or less bad alternative answers include: 1. Just document the current behavior. 2. On Windows, force a SIGHUP processing cycle if we're asked to execute the view and we see that no state data exists yet. This would have the result that the current backend might adopt settings that no other session has yet, which is not so great but might be tolerable. 3. Force a SIGHUP processing cycle but don't actually apply any of the values. This would be pretty messy though, especially if you wanted it to produce results exactly like the normal case so far as detection of incorrect values goes. I don't think this is a good idea; it's going to be too prone to corner-case bugs. 4. Revert the pg_file_settings patch altogether until the author comes up with a more portable implementation. Thoughts? As I said, I'm not volunteering to fix this. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Solaris testers wanted for strxfrm() behavior
convert_string_datum() says:
/*
* Note: originally we guessed at a suitable output buffer
size, and
* only needed to call strxfrm twice if our guess was too small.
* However, it seems that some versions of Solaris have buggy
strxfrm
* that can write past the specified buffer length in that
scenario.
* So, do it the dumb way for portability.
That arrived in commit 59d9a37, and I think this is the background:
http://www.postgresql.org/message-id/flat/3224.1020394...@sss.pgh.pa.us
PostgreSQL 9.5 adds a strxfrm() call in bttext_abbrev_convert(), which does
not account for the Solaris bug. I wish to determine whether that bug is
still relevant today. If you have access to Solaris with the is_IS.ISO8859-1
locale installed (or root access to install it), please compile and run the
attached test program on each Solaris version you have available. Reply here
with the program's output. I especially need a report from Solaris 10, but
reports from older and newer versions are valuable. Thanks.
Here is the output on OmniOS r151006, which does not have the bug:
SunOS ip-10-152-178-106.ec2.internal 5.11 omnios-b281e50 i86pc i386 i86xpv
locale is_IS.ISO8859-1: strxfrm returned 212; last modified byte at 58 (0x0)
locale is_IS.ISO8859-1: strxfrm returned 212; last modified byte at 58 (0x0)
locale : strxfrm returned 264; last modified byte at 58 (0x0)
#include locale.h
#include stdio.h
#include stdlib.h
#include string.h
void t(const char *locale, int canary)
{
char buf[1024];
size_t ret;
int i;
if (setlocale(LC_ALL, locale) == NULL)
printf(setlocale(\%s\) failed\n, locale);
memset(buf, canary, sizeof(buf)); buf[0] = canary - 1;
ret = strxfrm(buf + 1, pg_amop_opc_strategy_index, 58);
for (i = sizeof(buf) - 1; i = 0 buf[i] == canary; --i)
;
printf(locale \%s\: strxfrm returned %d; last modified byte at %d
(0x%hhx)\n,
locale, (int) ret, i, buf[i]);
}
int main(int argc, char **argv)
{
system(uname -a);
t(is_IS.ISO8859-1, 0x7F);
t(is_IS.ISO8859-1, 0x7E);
t(, 0x7F);
return 0;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Semantics of pg_file_settings view
Robert Haas robertmh...@gmail.com writes: On Fri, Jun 26, 2015 at 4:02 PM, Tom Lane t...@sss.pgh.pa.us wrote: Combining this with my idea about preserving the ConfigVariable list, I'm thinking that it would be a good idea for ProcessConfigFile() to run in a context created for the purpose of processing the config files, rather than blindly using the caller's context, which is likely to be a process-lifespan context and thus not a good place to leak in. We could keep this context around until the next SIGHUP event, so that the ConfigVariable list remains available, and then destroy it and replace it with the next ProcessConfigFile's instance of the context. In this way, any leakage in the processing code could not accumulate over multiple SIGHUPs, and so it would be certain to remain fairly negligible. That seems like a nice idea. Attached is a WIP version of this idea. It lacks docs, and there is one further API change I'd like to discuss, but what is there so far is: 1. It implements the above idea of doing SIGHUP work in a dedicated context that gets flushed at the next SIGHUP, and using the ConfigVariables list directly as the source data for the pg_file_settings view. 2. It adds an error message field to struct ConfigVariable, and a corresponding column to the pg_file_settings view, allowing problems to be reported. Some examples of what it can do: Normal case with an initdb-generated postgresql.conf: regression=# select * from pg_file_settings; sourcefile| sourceline | seqno | name| setting | error -++---+++--- /home/postgres/testversion/data/postgresql.conf | 64 | 1 | max_connections| 100| /home/postgres/testversion/data/postgresql.conf |116 | 2 | shared_buffers | 128MB | /home/postgres/testversion/data/postgresql.conf |131 | 3 | dynamic_shared_memory_type | posix | /home/postgres/testversion/data/postgresql.conf |446 | 4 | log_timezone | US/Eastern | /home/postgres/testversion/data/postgresql.conf |533 | 5 | datestyle | iso, mdy | /home/postgres/testversion/data/postgresql.conf |535 | 6 | timezone | US/Eastern | /home/postgres/testversion/data/postgresql.conf |548 | 7 | lc_messages| C | /home/postgres/testversion/data/postgresql.conf |550 | 8 | lc_monetary| C | /home/postgres/testversion/data/postgresql.conf |551 | 9 | lc_numeric | C | /home/postgres/testversion/data/postgresql.conf |552 |10 | lc_time | C | /home/postgres/testversion/data/postgresql.conf |555 |11 | default_text_search_config | pg_catalog.english | (11 rows) postgresql.conf is not there/not readable: regression=# select * from pg_file_settings; sourcefile | sourceline | seqno | name | setting | error ++---+--+-+--- | 0 | 1 | | | could not open file /home/postgres/testversion/data/postgresql.conf (1 row) Bad include_dir entry: sourcefile| sourceline | seqno | name| setting | error -++---+++ /home/postgres/testversion/data/postgresql.conf | 64 | 1 | max_connections| 100| /home/postgres/testversion/data/postgresql.conf |116 | 2 | shared_buffers | 128MB | /home/postgres/testversion/data/postgresql.conf |131 | 3 | dynamic_shared_memory_type | posix | /home/postgres/testversion/data/postgresql.conf |446 | 4 | log_timezone | US/Eastern | /home/postgres/testversion/data/postgresql.conf |533 | 5 | datestyle | iso, mdy | /home/postgres/testversion/data/postgresql.conf |535 | 6 | timezone | US/Eastern | /home/postgres/testversion/data/postgresql.conf |548 | 7 | lc_messages| C | /home/postgres/testversion/data/postgresql.conf |550 | 8 | lc_monetary| C |
Re: [HACKERS] Removing SSL renegotiation (Was: Should we back-patch SSL renegotiation fixes?)
On 2015-06-27 12:10:49 -0400, Tom Lane wrote: Andres Freund and...@anarazel.de writes: On 2015-06-27 15:07:05 +0900, Michael Paquier wrote: +1 for removing on master and just disabling on back-branches. The problem with that approach is that it leaves people hanging in the dry if they've uncommented the default value, or changed it. That doesn't seem nice to me. I think at least 99% of the people who are using a nondefault value of ssl_renegotiation_limit are using zero and so would have no problem with this at all. Possibly 100% of them; there's not really much use-case for changing from 512MB to some other nonzero value, is there? While still at 2ndq I've seen some increase it to nonzero values to cope with the connection breaks. 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] Solaris testers wanted for strxfrm() behavior
On Sat, Jun 27, 2015 at 9:51 AM, Noah Misch n...@leadboat.com wrote: PostgreSQL 9.5 adds a strxfrm() call in bttext_abbrev_convert(), which does not account for the Solaris bug. I wish to determine whether that bug is still relevant today. If you have access to Solaris with the is_IS.ISO8859-1 locale installed (or root access to install it), please compile and run the attached test program on each Solaris version you have available. Reply here with the program's output. I especially need a report from Solaris 10, but reports from older and newer versions are valuable. Thanks. I did consider this. Sorry, but I must question the point of worrying about an ancient bug in Solaris. When you have to worry about a standard library function blithely writing past the end of a buffer, when its C89 era interface must be passed the size of said buffer, where does it end? This is not a portability concern, like checking for an INT_MAX return value from strxfrm() on Windows. The Solaris issue is patently a bug that existed in some particular release of the Solaris C stdlib many years ago. The documented behavior of strxfrm() in that library was surely not We ignore the bufsize argument. -- 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_file_settings view vs. Windows
Michael Paquier michael.paqu...@gmail.com writes: On Sun, Jun 28, 2015 at 8:20 AM, Tom Lane t...@sss.pgh.pa.us wrote: Yeah, exactly. Unfortunately I see no way to add a useful test, at least not one that will work in installcheck mode. There's no way to predict what will be in the view in that case. Even for make check, the output would be pretty darn environment-dependent. And also because this patch had no review input regarding Windows and EXEC_BACKEND. I would suggest pinging the author (just did so), waiting for a fix a bit, and move on with 4. if nothing happens. Well, mumble. After playing with this for a bit, I'm fairly convinced that it offers useful functionality, especially with the error-reporting additions I've proposed. Right now, there is no easy way to tell whether a SIGHUP has worked, or why not if not, unless you have access to the postmaster log. So I think there's definite usefulness here for remote-administration scenarios. So I kinda think that alternative 1 (document the Windows deficiency) is better than having no such functionality at all. Obviously a proper fix would be better yet, but that's something that could be rolled in later. We usually require that a patch includes support for Windows as a requirement (see for example discussions about why pg_fincore in not a contrib module even if it overlaps a bit with pg_prewarm), why would this patch have a different treatment? Agreed, but it was evidently not obvious to anyone that there was a portability issue in this code, else we'd have resolved the issue before it got committed. As a thought experiment, what would happen if we'd not noticed this issue till post-release, which is certainly not implausible? Also, there are multiple pre-existing minor bugs (the leakage problem I mentioned earlier, and some other things I've found while hacking on the view patch) that we would have to deal with in some other way if we revert now. I'd just as soon not detangle that. 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_file_settings view vs. Windows
I noticed that in EXEC_BACKEND builds (ie, Windows) the pg_file_settings view doesn't act as its author presumably intended. Specifically, it reads as empty until/unless the current session processes a SIGHUP event. This is because before that happens, it's depending on having inherited the state data from the postmaster via fork(), which of course does not happen with EXEC_BACKEND. This applies to both the committed version and my proposed rewrite. AFAICS the only correct fix would be to add the pg_file_settings state data to the set of data dumped and reloaded through write_nondefault_variables/read_nondefault_variables. That would add a fair amount of code, and it might hurt backend startup time more than the feature is worth. In any case, I'm not volunteering to do it. More or less bad alternative answers include: 1. Just document the current behavior. 2. On Windows, force a SIGHUP processing cycle if we're asked to execute the view and we see that no state data exists yet. This would have the result that the current backend might adopt settings that no other session has yet, which is not so great but might be tolerable. 3. Force a SIGHUP processing cycle but don't actually apply any of the values. This would be pretty messy though, especially if you wanted it to produce results exactly like the normal case so far as detection of incorrect values goes. I don't think this is a good idea; it's going to be too prone to corner-case bugs. 4. Revert the pg_file_settings patch altogether until the author comes up with a more portable implementation. Thoughts? As I said, I'm not volunteering to fix this. I'm just wondering why we did not catch this earlier. If this is because threre's no regression test case for pg_file_settings view, we should add along with any of solutions above (of course except #4) IMO. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent 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_file_settings view vs. Windows
Tatsuo Ishii is...@postgresql.org writes: I noticed that in EXEC_BACKEND builds (ie, Windows) the pg_file_settings view doesn't act as its author presumably intended. Specifically, it reads as empty until/unless the current session processes a SIGHUP event. I'm just wondering why we did not catch this earlier. If this is because threre's no regression test case for pg_file_settings view, Yeah, exactly. Unfortunately I see no way to add a useful test, at least not one that will work in installcheck mode. There's no way to predict what will be in the view in that case. Even for make check, the output would be pretty darn environment-dependent. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Refactoring pgbench.c
Main pgbench logic consists of single file pgbench.c which is 4036 lines of code as of today. This is not a small number and I think it would be nice if it is divided into smaller files because it will make it easier to maintain, add or change features of pgbench. I will come up with an idea how to split pgbench.c later. In the mean time I attached a call graph of pgbench.c generated by egypt, which we could get a basic idea how to split and modularize pgbench.c. BTW, while looking at pgbench.c I noticed subtle coding problems: 1) strtoint64() should be decalred as static 2) static void agg_vals_init(AggVals *aggs, instr_time start) static and void should be one line, rather than separate 2 lines according to our coding style. I will commit fix for these if there's no objection. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent 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_file_settings view vs. Windows
On Sun, Jun 28, 2015 at 8:20 AM, Tom Lane t...@sss.pgh.pa.us wrote: Tatsuo Ishii is...@postgresql.org writes: I noticed that in EXEC_BACKEND builds (ie, Windows) the pg_file_settings view doesn't act as its author presumably intended. Specifically, it reads as empty until/unless the current session processes a SIGHUP event. I'm just wondering why we did not catch this earlier. If this is because threre's no regression test case for pg_file_settings view, Yeah, exactly. Unfortunately I see no way to add a useful test, at least not one that will work in installcheck mode. There's no way to predict what will be in the view in that case. Even for make check, the output would be pretty darn environment-dependent. And also because this patch had no review input regarding Windows and EXEC_BACKEND. I would suggest pinging the author (just did so), waiting for a fix a bit, and move on with 4. if nothing happens. We usually require that a patch includes support for Windows as a requirement (see for example discussions about why pg_fincore in not a contrib module even if it overlaps a bit with pg_prewarm), why would this patch have a different treatment? -- 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] Solaris testers wanted for strxfrm() behavior
Peter Geoghegan p...@heroku.com writes: On Sat, Jun 27, 2015 at 9:51 AM, Noah Misch n...@leadboat.com wrote: PostgreSQL 9.5 adds a strxfrm() call in bttext_abbrev_convert(), which does not account for the Solaris bug. I wish to determine whether that bug is still relevant today. If you have access to Solaris with the is_IS.ISO8859-1 locale installed (or root access to install it), please compile and run the attached test program on each Solaris version you have available. Reply here with the program's output. I especially need a report from Solaris 10, but reports from older and newer versions are valuable. Thanks. I did consider this. Sorry, but I must question the point of worrying about an ancient bug in Solaris. I think the point of Noah's query is to find out whether ancient is an accurate description. If indeed nothing newer than Solaris 8 exhibits the bug, I'd be okay with not working around it, but otherwise we have some decisions to make. Also, the post Noah dug up was merely the oldest description of the problem. I believe what prompted us to actually change the code was some reports in July 2003: http://www.postgresql.org/message-id/flat/56510aaef435d240958d1ce8c6b1770a016d2...@mailc03.aurigin.com http://www.postgresql.org/message-id/flat/56510aaef435d240958d1ce8c6b1770a016d2...@mailc03.aurigin.com (the archive threading here is pretty crappy, but you can poke around among posts of similar date) Those reports suggest that the problem was observable in many non-C locales on Solaris 8, not only Icelandic. 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] Solaris testers wanted for strxfrm() behavior
27.06.2015, 19:51, Noah Misch kirjoitti: convert_string_datum() says: /* * Note: originally we guessed at a suitable output buffer size, and * only needed to call strxfrm twice if our guess was too small. * However, it seems that some versions of Solaris have buggy strxfrm * that can write past the specified buffer length in that scenario. * So, do it the dumb way for portability. That arrived in commit 59d9a37, and I think this is the background: http://www.postgresql.org/message-id/flat/3224.1020394...@sss.pgh.pa.us PostgreSQL 9.5 adds a strxfrm() call in bttext_abbrev_convert(), which does not account for the Solaris bug. I wish to determine whether that bug is still relevant today. If you have access to Solaris with the is_IS.ISO8859-1 locale installed (or root access to install it), please compile and run the attached test program on each Solaris version you have available. Reply here with the program's output. I especially need a report from Solaris 10, but reports from older and newer versions are valuable. Thanks. Here is the output on OmniOS r151006, which does not have the bug: SunOS ip-10-152-178-106.ec2.internal 5.11 omnios-b281e50 i86pc i386 i86xpv locale is_IS.ISO8859-1: strxfrm returned 212; last modified byte at 58 (0x0) locale is_IS.ISO8859-1: strxfrm returned 212; last modified byte at 58 (0x0) locale : strxfrm returned 264; last modified byte at 58 (0x0) SunOS larry 5.10 Generic_147147-26 sun4u sparc SUNW,Sun-Fire-V215 locale is_IS.ISO8859-1: strxfrm returned 216; last modified byte at 58 (0x0) locale is_IS.ISO8859-1: strxfrm returned 216; last modified byte at 58 (0x0) locale : strxfrm returned 26; last modified byte at 27 (0x0) / Oskari -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_trgm version 1.2
This patch implements version 1.2 of contrib module pg_trgm. This supports the triconsistent function, introduced in version 9.4 of the server, to make it faster to implement indexed queries where some keys are common and some are rare. I've included the paths to both upgrade and downgrade between 1.1 and 1.2, although after doing so you must close and restart the session before you can be sure the change has taken effect. There is no change to the on-disk index structure This shows the difference it can make in some cases: create extension pg_trgm version 1.1; create table foo as select md5(random()::text)|| case when random()0.05 then 'lmnop' else '123' end || md5(random()::text) as bar from generate_series(1,1000); create index on foo using gin (bar gin_trgm_ops); --some queries alter extension pg_trgm update to 1.2; --close, reopen, more queries select count(*) from foo where bar like '%12344321lmnabcddd%'; V1.1: Time: 1743.691 ms --- after repeated execution to warm the cache V1.2: Time: 2.839 ms --- after repeated execution to warm the cache You could get the same benefit just by increasing MAX_MAYBE_ENTRIES (in core) from 4 to some higher value (which it probably should be anyway, but there will always be a case where it needs to be higher than you can afford it to be, so a real solution is needed). I wasn't sure if this should be a new version of pg_trgm or not, because there is no user visible change other than to performance. But there may be some cases where it results in performance reduction and so it is nice to provide options. Also, I'd like to use it in a back-branch, so versions seems to be the right way to go there. There is a lot of code duplication between the binary consistent function and the ternary one. I thought it the duplication was necessary in order to support both 1.1 and 1.2 from the same code base. There may also be some gains in the similarity and regex cases, but I didn't really analyze those for performance. I've thought about how to document this change. Looking to other example of other contrib modules with multiple versions, I decided that we don't document them, other than in the release notes. The same patch applies to 9.4 code with a minor conflict in the Makefile, and gives benefits there as well. Cheers, Jeff pg_trgm_1_2_v001.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
[HACKERS] proposal: condition blocks in psql
Hi I am thinking about simplifying a deployment some multiversion PostgreSQL extensions, and scripts. With current possibilities, we have to use DO statement, what is not optimal or possible in some use cases. The implementation of condition block (possible nested) is very simple. The proposed syntax of new psql commands \if_ver_eq 9.2 ... \else \endif \if_ver_gt 9.2 \if_ver_ge 9.2 \if_ver_le 9.2 \if_ver_lt 9.2 minor versions can be supported too \if_ver_ge 9.2.0 \endif \if_def psqlvariable \if_eq psqlvariable \if_ne psqlvariable What do you thinking about it? Regards Pavel
Re: [HACKERS] drop/truncate table sucks for large values of shared buffers
On Sat, Jun 27, 2015 at 8:06 PM, Gurjeet Singh gurj...@singh.im wrote: On Fri, Jun 26, 2015 at 9:45 PM, Amit Kapila amit.kapil...@gmail.com wrote: Attached patch implements the above idea and I found that performance doesn't dip much with patch even with large value of shared_buffers. I have also attached script and sql file used to take performance data. +1 for the effort to improve this. Thanks. With your technique added, there are 3 possible ways the search can happen a) Scan NBuffers and scan list of relations, b) Scan NBuffers and bsearch list of relations, and c) Scan list of relations and then invalidate blocks of each fork from shared buffers. Would it be worth it finding one technique that can serve decently from the low-end shared_buffers to the high-end. Yes, it would be great if we could have any such technique, but in absence of that current optimization would suffice the need unless there are any loop-holes in it. On patch: There are multiple naming styles being used in DropForkSpecificBuffers(); my_name and myName. Given this is a new function, it'd help to be consistent. Thanks for suggestions, I will improve the patch on those lines if there is no problem with idea at broad level. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] pg_file_settings view vs. Windows
I'm just wondering why we did not catch this earlier. If this is because threre's no regression test case for pg_file_settings view, Yeah, exactly. Unfortunately I see no way to add a useful test, at least not one that will work in installcheck mode. There's no way to predict what will be in the view in that case. Even for make check, the output would be pretty darn environment-dependent. Is there anything like this (9.5 features not tested on Windows) other than pg_file_settings? Maybe SRA OSS could help in testing such features on Windows. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: condition blocks in psql
The proposed syntax of new psql commands \if_ver_eq 9.2 ... \else \endif What do you thinking about it? Couldn't this kind of thing be done directly with PL/pgSQL? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Solaris testers wanted for strxfrm() behavior
On Sat, Jun 27, 2015 at 7:14 PM, Tom Lane t...@sss.pgh.pa.us wrote: I think the point of Noah's query is to find out whether ancient is an accurate description. If indeed nothing newer than Solaris 8 exhibits the bug, I'd be okay with not working around it, but otherwise we have some decisions to make. Even Solaris 9 is EOL. Also, the post Noah dug up was merely the oldest description of the problem. I believe what prompted us to actually change the code was some reports in July 2003: http://www.postgresql.org/message-id/flat/56510aaef435d240958d1ce8c6b1770a016d2...@mailc03.aurigin.com You said it yourself at the time -- why trust the strxfrm() implementation when a NULL pointer is passed? It may have worked on someone's machine in 2003, but that isn't a very good reason. It was never a documented part of the interface that this fails or that works; how could it be? This Solaris strxfrm() issue is (in the simplest and least contentious sense) a bug. It is not a portability issue. Someone made a mistake, and most likely the mistake was corrected in the next point release. That is the only logical explanation I can see. I wouldn't say that adding defenses to workaround serious bugs in a C stdlib is something we should never do, but ISTM that the standard for undertaking that ought to be very high. BTW, unlike the author of convert_string_datum(), I think it's okay that glibc sometimes returns different values with strxfrm() calls on the same string (based on whether or not the passed-in buffer is actually big enough to store the transformed string) -- that is actually allowed by the standard, I think. In other words, Solaris 8 has the only actually buggy strxfrm() of the cases convert_string_datum() enumerates, since AFAICT the standard doesn't promise that the strxfrm() return value need be exact, only sufficient (this leeway seems useful to me). -- 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] Semantics of pg_file_settings view
On Sat, Jun 27, 2015 at 7:19 PM, Tom Lane t...@sss.pgh.pa.us wrote: Amit Kapila amit.kapil...@gmail.com writes: On Fri, Jun 26, 2015 at 9:47 PM, Tom Lane t...@sss.pgh.pa.us wrote: What we evidently need to do is fix things so that the pg_file_settings data gets captured before we suppress duplicates. The simplest change would be to move the whole thing to around line 208 in guc-file.l, just after the stanza that loads PG_AUTOCONF_FILENAME. Or you could argue that the approach is broken altogether, and that we should capture the data while we read the files, so that you have some useful data in the view even if ParseConfigFile later decides there's a syntax error. I'm actually thinking maybe we should flush that data-capturing logic altogether in favor of just not deleting the ConfigVariable list data structure, and generating the view directly from that data structure. Idea for generating view form ConfigVariable list sounds good, but how will it preserve the duplicate entries in the list assuming either we need to revert the original fix (e3da0d4d1) or doing the same in loop where we set GUC_IS_IN_FILE? I'm thinking of adding an ignore boolean flag to ConfigVariable, which the GUC_IS_IN_FILE loop would set in ConfigVariables that are superseded by later list entries. Then the GUC-application loop would just skip those entries. This would be good because the flag could be displayed somehow in the pg_file_settings view, whereas right now you have to manually check for duplicates. Sounds good way to deal with this problem. Keeping removal of duplicate items in ParseConfigFp() has the advantage that it will work for all other places from where ParseConfigFp() is called, though I am not sure if today that is required. I don't think it is; if it were, we'd have had other complaints about that, considering that 9.4.0 is the *only* release we've ever shipped that suppressed duplicates right inside ParseConfigFp(). I would in fact turn that argument on its head, and state that Fujii-san's patch was probably ill-conceived because it implicitly assumes that duplicate suppression is okay for every caller of ParseConfigFp. I have implemented that patch, so if you see any problem's with that approach, Fujji-san is not right person to blame. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] pg_file_settings view vs. Windows
On Sun, Jun 28, 2015 at 10:40 AM, Tom Lane t...@sss.pgh.pa.us wrote: Michael Paquier michael.paqu...@gmail.com writes: On Sun, Jun 28, 2015 at 8:20 AM, Tom Lane t...@sss.pgh.pa.us wrote: Yeah, exactly. Unfortunately I see no way to add a useful test, at least not one that will work in installcheck mode. There's no way to predict what will be in the view in that case. Even for make check, the output would be pretty darn environment-dependent. And also because this patch had no review input regarding Windows and EXEC_BACKEND. I would suggest pinging the author (just did so), waiting for a fix a bit, and move on with 4. if nothing happens. Well, mumble. After playing with this for a bit, I'm fairly convinced that it offers useful functionality, especially with the error-reporting additions I've proposed. Right now, there is no easy way to tell whether a SIGHUP has worked, or why not if not, unless you have access to the postmaster log. So I think there's definite usefulness here for remote-administration scenarios. So I kinda think that alternative 1 (document the Windows deficiency) is better than having no such functionality at all. Obviously a proper fix would be better yet, but that's something that could be rolled in later. We usually require that a patch includes support for Windows as a requirement (see for example discussions about why pg_fincore in not a contrib module even if it overlaps a bit with pg_prewarm), why would this patch have a different treatment? Agreed, but it was evidently not obvious to anyone that there was a portability issue in this code, else we'd have resolved the issue before it got committed. As a thought experiment, what would happen if we'd not noticed this issue till post-release, which is certainly not implausible? Also, there are multiple pre-existing minor bugs (the leakage problem I mentioned earlier, and some other things I've found while hacking on the view patch) that we would have to deal with in some other way if we revert now. I'd just as soon not detangle that. Thank you for bug report. I have not came up with portable idea yet, but I will deal with this problem immediately. If I couldn't come up with better solution, I'd like to propose #1 idea. But it would be unavoidable to be revert it if there are any reason for Windows support. Regards, -- Sawada Masahiko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] drop/truncate table sucks for large values of shared buffers
On Sat, Jun 27, 2015 at 7:40 PM, Tom Lane t...@sss.pgh.pa.us wrote: Amit Kapila amit.kapil...@gmail.com writes: I have looked into it and found that the main reason for such a behaviour is that for those operations it traverses whole shared_buffers and it seems to me that we don't need that especially for not-so-big tables. We can optimize that path by looking into buff mapping table for the pages that exist in shared_buffers for the case when table size is less than some threshold (say 25%) of shared buffers. I don't like this too much because it will fail badly if the caller is wrong about the maximum possible page number for the table, which seems not exactly far-fetched. (For instance, remember those kernel bugs we've seen that cause lseek to lie about the EOF position?) Considering we already have exclusive lock while doing this operation and nobody else can perform write on this file, won't closing and opening it again would avoid such problems. In patch that is already done (smgrexists()) for 2 kind of forks and can be done for the third kind as well. It also offers no hope of a fix for the other operations that scan the whole buffer pool, such as DROP TABLESPACE and DROP DATABASE. True, but it is not foreclosing if any body has idea to optimize those paths. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
