Re: [HACKERS] Displaying accumulated autovacuum cost
On 09/26/2011 05:58 AM, Shigeru Hanada wrote: > * Local variables added by the patch (secs, usecs, write_rate and > endtime) can be moved into narrower scope. > * Initializing starttime to zero seems unnecessary. > Setting starttime to 0 is already in the code; the change made to that line was to add endtime, which is not initialized. You may be right that initializing it isn't necessary, but I'm sure not going to touch that part of the working code. You're right about the the local variables; they were placed to look like the surrounding code rather than to be as local as possible. I'm not sure if all the PostgreSQL code is completely consistent here; a quick survey shows examples of both "put all the variables at the top" and "make variables as local to blocks as possible" styles. I don't know that it really makes any difference with modern compilers, either. I'm sure someone else will have a stronger opinion on this subject now that I've trolled the list for one by writing this. > In addition, documents about how to use the statistics would be > necessary, maybe in "23.1.5. The Autovacuum Daemon". > I'll set the status of this patch to waiting-on-author. Would you rebase > the patch and post it again? > I didn't do that because there's not really much documentation at this level of detail yet--how to interpret all the information in the logs. That's an open-ended bit of work; there's a lot more that could be written on this topic than the docs have right now. It's probably worth pointing out that this specific info is now in the logs, though, given that people might not notice it otherwise. I'll see what I can do about that. As a general FYI, rebasing is normally requested only when the existing patch doesn't apply anymore. If "patch" or "git apply" can consume it, complaints about code shifting around isn't by itself enough reason to ask for an updated patch. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- 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: Force strings passed to and from plperl to be in UTF8 encoding.
On Tue, Oct 4, 2011 at 23:46, Amit Khandekar
wrote:
> On 4 October 2011 22:57, Alex Hunsaker wrote:
>> On Tue, Oct 4, 2011 at 03:09, Amit Khandekar
>> wrote:
>>> On 4 October 2011 14:04, Alex Hunsaker wrote:
On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
wrote:
> WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
> utf8_str, so pg_verify_mbstr_len() will not get called. [...]
Consider a latin1 database where utf8_str was a string of ascii
characters. [...]
>>
[Patch] Look ok to you?
>>>
>>> + if(GetDatabaseEncoding() == PG_UTF8)
>>> + pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
>>>
>>> In your patch, the above will again skip mb-validation if the database
>>> encoding is SQL_ASCII. Note that in pg_do_encoding_conversion returns
>>> the un-converted string even if *one* of the src and dest encodings is
>>> SQL_ASCII.
>>
>> *scratches head* I thought the point of SQL_ASCII was no encoding
>> conversion was done and so there would be nothing to verify.
>>
>> Ahh I see looks like pg_verify_mbstr_len() will make sure there are no
>> NULL bytes in the string when we are a single byte encoding.
>>
>>> I think :
>>> if (ret == utf8_str)
>>> + {
>>> + pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
>>> ret = pstrdup(ret);
>>> + }
>>>
>>> This (ret == utf8_str) condition would be a reliable way for knowing
>>> whether pg_do_encoding_conversion() has done the conversion at all.
>>
>> Yes. However (and maybe im nitpicking here), I dont see any reason to
>> verify certain strings twice if we can avoid it.
>>
>> What do you think about:
>> + /*
>> + * when we are a PG_UTF8 or SQL_ASCII database
>> pg_do_encoding_conversion()
>> + * will not do any conversion or verification. we need to do it
>> manually instead.
>> + */
>> + if( GetDatabaseEncoding() == PG_UTF8 ||
>> GetDatabaseEncoding() == SQL_ASCII)
>> + pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
>>
>
> You mean the final changes in plperl_helpers.h would look like
> something like this right? :
>
> static inline char *
> utf_u2e(const char *utf8_str, size_t len)
> {
> char *ret = (char *) pg_do_encoding_conversion((unsigned
> char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());
>
> if (ret == utf8_str)
> + {
> + if (GetDatabaseEncoding() == PG_UTF8 ||
> + GetDatabaseEncoding() == PG_SQL_ASCII)
> + {
> + pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
> + }
> +
> ret = pstrdup(ret);
> + }
> return ret;
> }
Yes.
> Yeah I am ok with that. It's just an additional check besides (ret ==
> utf8_str) to know if we really require validation.
>
--
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] Action requested - Application Softblock implemented | Issue report ID341057
On 04.10.2011 22:46, Seiko Ishida (MP Tech Consulting LLC) wrote: Our team drives the bug notification activity with our valued Windows partners. This email is to notify you that PostgreSQL's application/driver experienced compatibility issue(s) during internal Microsoft testing and has been blocked. Please note that this block may already be in the latest Windows Developer Preview build so your prompt attention to the issue is much appreciated. ... Here are the details of the Softblock implementations: Compatibility Issue: Product name: PostgreSQL 8.2 Version 8.2 is quite old, and the community support for it will end in December. I don't think anyone cares if it works on Windows 8. If you could test with PostgreSQL 9.1, that would be great. -- Heikki Linnakangas 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] [PATCH] Unremovable tuple monitoring
On 10/04/2011 03:45 PM, Royce Ausburn wrote: I think I get this stats stuff now. Unless someone here thinks it's too hard for a new postgres dev's 2nd patch, I could take a stab. I might take a look at it tonight to get a feel for how hard, and what stats we could collect. I'll start a new thread for discussion. Adding statistics and good monitoring points isn't hard to do, once you figure out how the statistics messaging works. From looking at your patch, you seem to be over that part of the learning curve already. The most time consuming part for vacuum logging patches is setting up the test cases and waiting for them to execute. If you can provide a script that does that, it will aid in getting review off to a goo start. Basically, whatever you build to test the patch, you should think about packaging into a script you can hand to a reviewer/committer. Normal approach is to build a test data set with something like generate_series, then create the situation the patch is supposed to log. Just to clarify what Robert was suggesting a little further, good practice here is just to say "this patch needs a catversion bump", but not actually do it. Committers should figure that out even if you don't mention it, but sometimes a goof is made; a little reminder doesn't hurt. I'm not sure what my next step should be. I've added this patch to the open commit fest -- is that all for now until the commit fest begins review? Basically, we'll get to it next month. I have my own autovacuum logging stuff I'm working on that I expect to slip to that one too, so I can easily take on reviewing yours then. I just fixed the entry in the CF app to follow convention by listing your full name. Main feedback for now on the patch is that few people ever use pg_stat_all_tables. The new counter needs to go into pg_stat_user_tables and pg_stat_sys_tables if it's going to be visible to the people who are most likely to need it. I updated the name of the patch on the CommitFest to read "Unremovable tuple count on pg_stat_*_tables" so the spec here is more clear. I'd suggest chewing on the rest of your ideas, see what else falls out of this, and just make sure to submit another update just before the next CF starts. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- 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: Force strings passed to and from plperl to be in UTF8 encoding.
On 4 October 2011 22:57, Alex Hunsaker wrote:
> On Tue, Oct 4, 2011 at 03:09, Amit Khandekar
> wrote:
>> On 4 October 2011 14:04, Alex Hunsaker wrote:
>>> On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
>>> wrote:
>>>
WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
utf8_str, so pg_verify_mbstr_len() will not get called. [...]
>>>
>>> Consider a latin1 database where utf8_str was a string of ascii
>>> characters. [...]
>
>>> [Patch] Look ok to you?
>>>
>>
>> + if(GetDatabaseEncoding() == PG_UTF8)
>> + pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
>>
>> In your patch, the above will again skip mb-validation if the database
>> encoding is SQL_ASCII. Note that in pg_do_encoding_conversion returns
>> the un-converted string even if *one* of the src and dest encodings is
>> SQL_ASCII.
>
> *scratches head* I thought the point of SQL_ASCII was no encoding
> conversion was done and so there would be nothing to verify.
>
> Ahh I see looks like pg_verify_mbstr_len() will make sure there are no
> NULL bytes in the string when we are a single byte encoding.
>
>> I think :
>> if (ret == utf8_str)
>> + {
>> + pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
>> ret = pstrdup(ret);
>> + }
>>
>> This (ret == utf8_str) condition would be a reliable way for knowing
>> whether pg_do_encoding_conversion() has done the conversion at all.
>
> Yes. However (and maybe im nitpicking here), I dont see any reason to
> verify certain strings twice if we can avoid it.
>
> What do you think about:
> + /*
> + * when we are a PG_UTF8 or SQL_ASCII database pg_do_encoding_conversion()
> + * will not do any conversion or verification. we need to do it
> manually instead.
> + */
> + if( GetDatabaseEncoding() == PG_UTF8 ||
> GetDatabaseEncoding() == SQL_ASCII)
> + pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
>
You mean the final changes in plperl_helpers.h would look like
something like this right? :
static inline char *
utf_u2e(const char *utf8_str, size_t len)
{
char *ret = (char *) pg_do_encoding_conversion((unsigned
char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());
if (ret == utf8_str)
+ {
+ if (GetDatabaseEncoding() == PG_UTF8 ||
+ GetDatabaseEncoding() == PG_SQL_ASCII)
+ {
+ pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
+ }
+
ret = pstrdup(ret);
+ }
return ret;
}
Yeah I am ok with that. It's just an additional check besides (ret ==
utf8_str) to know if we really require validation.
--
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] timezone buglet?
daveg writes: > Postgresql 9.0.4 has the timezone: > America/Blanc-Sablon > However other sources seem to spell this with an underscore instead of dash: > America/Blanc_Sablon I don't know what "other sources" you're consulting, but "Blanc-Sablon" is the way it appears in the Olson timezone database, and that's what we follow. We're not going to get into the business of editorializing on their information. If you want to fool with it locally, look into the .../share/timezone/ directory. 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] checkpoints are duplicated even while the system is idle
Hi, While the system is idle, we skip duplicate checkpoints for some reasons. But when wal_level is set to hot_standby, I found that checkpoints are wrongly duplicated even while the system is idle. The cause is that XLOG_RUNNING_XACTS WAL record always follows CHECKPOINT one when wal_level is set to hot_standby. So the subsequent checkpoint wrongly thinks that there is inserted record (i.e., XLOG_RUNNING_XACTS record) since the start of the last checkpoint, the system is not idle, and this checkpoint cannot be skipped. Is this intentional behavior? Or a bug? Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] timezone buglet?
Postgresql 9.0.4 has the timezone: America/Blanc-Sablon However other sources seem to spell this with an underscore instead of dash: America/Blanc_Sablon It appears that beside 'America/Blanc_Sablon', other multi-word timezones are spelled with underscore. For example: 'Australia/Broken_Hill', 'Asia/Ho_chi_minh', 'America/Los_Angeles', and so on. Two questions: Is this correct as is, or is it wrong in 9.0.4? And, should I have reported this somewhere else? Bugs? Err, three questions: I'm a little unclear on how the tz machinery works. Can I just update the name column in pg_timezones to fix it for now? Thanks -dg -- David Gould da...@sonic.net 510 536 1443510 282 0869 If simplicity worked, the world would be overrun with insects. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Separating bgwriter and checkpointer
2011/10/4 Simon Riggs : > The problem is the *same* one I fixed in v2, yet now I see I managed > to somehow exclude that fix from the earlier patch. Slap. Anyway, > fixed again now. Ah ok! I started reviewing the v4 patch version, this is my comments: Submission review === 1. The patch applies cleanly to current master (fa56a0c3e) but isn't in context diff format; Feature test == 1. Since I patched and make installed it, I can see the expected processes: writer and checkpointer; 2. I did the following tests with the following results: 2.1 Running a long time pgbench didn't emit any assertion failure or crash and the checkpoint works as before patch: LOG: checkpoint starting: xlog LOG: checkpoint complete: wrote 300 buffers (9.8%); 0 transaction log file(s) added, 0 removed, 0 recycled; write=26.103 s, sync=6.492 s, total=34.000 s; sync files=13, longest=4.684 s, average=0.499 s LOG: checkpoint starting: time LOG: checkpoint complete: wrote 257 buffers (8.4%); 0 transaction log file(s) added, 0 removed, 3 recycled; write=21.819 s, sync=9.610 s, total=32.076 s; sync files=7, longest=6.452 s, average=1.372 s 2.2 Forcing a checkpoint when filesystem has enough free space works fine while pgbench is running: LOG: checkpoint starting: immediate force wait LOG: checkpoint complete: wrote 1605 buffers (52.2%); 0 transaction log file(s) added, 0 removed, 2 recycled; write=0.344 s, sync=22.750 s, total=23.700 s; sync files=10, longest=15.586 s, average=2.275 s 2.3 Forcing a checkpoint when filesystem are full, works as expected: LOG: checkpoint starting: immediate force wait time ERROR: could not write to file "pg_xlog/xlogtemp.4380": Não há espaço disponível no dispositivo ERROR: checkpoint request failed HINT: Consult recent messages in the server log for details. STATEMENT: CHECKPOINT ; ... ERROR: could not extend file "base/16384/16405": wrote only 4096 of 8192 bytes at block 10 HINT: Check free disk space. STATEMENT: INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (69, 3, 609672, -3063, CURRENT_TIMESTAMP); PANIC: could not write to file "pg_xlog/xlogtemp.4528": Não há espaço disponível no dispositivo STATEMENT: END; LOG: server process (PID 4528) was terminated by signal 6: Aborted Then I freed some space and started it again and the server ran properly: LOG: database system was shut down at 2011-10-05 00:46:33 BRT LOG: database system is ready to accept connections LOG: autovacuum launcher started ... LOG: checkpoint starting: immediate force wait LOG: checkpoint complete: wrote 0 buffers (0.0%); 0 transaction log file(s) added, 0 removed, 0 recycled; write=0.000 s, sync=0.000 s, total=0.181 s; sync files=0, longest=0.000 s, average=0.000 s 2.2 Running a pgbench and interrupting postmaster a few seconds later, seems to work as expected, returning the output: ... cut ... LOG: statement: SELECT abalance FROM pgbench_accounts WHERE aid = 148253; ^CLOG: statement: UPDATE pgbench_tellers SET tbalance = tbalance + 934 WHERE tid = 85; DEBUG: postmaster received signal 2 LOG: received fast shutdown request LOG: aborting any active transactions FATAL: terminating connection due to administrator command FATAL: terminating connection due to administrator command ... cut ... LOG: disconnection: session time: 0:00:14.917 user=guedes database=bench host=[local] LOG: disconnection: session time: 0:00:14.773 user=guedes database=bench host=[local] DEBUG: server process (PID 1910) exited with exit code 1 DEBUG: server process (PID 1941) exited with exit code 1 LOG: shutting down LOG: checkpoint starting: shutdown immediate DEBUG: SlruScanDirectory invoking callback on pg_multixact/offsets/ DEBUG: SlruScanDirectory invoking callback on pg_multixact/members/ DEBUG: checkpoint sync: number=1 file=base/16384/16398 time=1075.227 msec DEBUG: checkpoint sync: number=2 file=base/16384/16406 time=16.832 msec DEBUG: checkpoint sync: number=3 file=base/16384/16397 time=12306.204 msec DEBUG: checkpoint sync: number=4 file=base/16384/16397.1 time=2122.141 msec DEBUG: checkpoint sync: number=5 file=base/16384/16406_fsm time=32.278 msec DEBUG: checkpoint sync: number=6 file=base/16384/16385_fsm time=11.248 msec DEBUG: checkpoint sync: number=7 file=base/16384/16388 time=11.083 msec DEBUG: checkpoint sync: number=8 file=base/16384/11712 time=11.314 msec DEBUG: checkpoint sync: number=9 file=base/16384/16397_vm time=11.103 msec DEBUG: checkpoint sync: number=10 file=base/16384/16385 time=19.308 msec DEBUG: attempting to remove WAL segments older than log file 0001 DEBUG: SlruScanDirectory invoking callback on pg_subtrans/ LOG: checkpoint complete: wrote 1659 buffers (54.0%); 0 transaction log file(s) added, 0 removed, 0 recycled; write=0.025 s, sync=15.617 s, total=15.898 s; sync files=10, longest=12.306 s, average=1.561 s LOG: database system is shut down
Re: [HACKERS] Inlining comparators as a performance optimisation
On Wed, Oct 5, 2011 at 2:49 AM, Bruce Momjian wrote: > Rather than parallelizing > the entire backend, I imagine adding threading or helper processes for > things like sorts, index scans, executor nodes, and stored procedure > languages. I expect final code to be 2-3 years in the future. I don't actually see it would be a big problem quicksort to start up a bunch of threads which do some of the work and go away when the tuplesort is done. As long as the code they're executing is well defined and limited to a small set of code that can be guaranteed to be thread-safe it should be reasonably simple. The problem is that in most of the Postgres core there aren't many places where much code fits that description. Even in tuplesort it can call out to arbitrary user-defined functions so we would need a way to mark which functions are thread-safe. Beyond integer and floating point comparisons it may not be much -- certainly not Numeric or strings due to detoasting... And then there are things like handling the various signals (including SI invalidations?) and so on. I agree that if we wanted to farm out entire plan nodes we would probably end up generating new partial plans which would be handed to other backend processes. That's not unlike what Oracle did with Parallel Query. But i'm a bit skeptical that we'll get much of that done in 2-3 years. The main use case for Parallel Query in Oracle is for partitioned tables -- and we haven't really polished that after how many years? -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Log crashed backend's query v2
On Wed, Oct 5, 2011 at 02:36, gabrielle wrote: > This review was compiled from a PDXPUG group review (Dan Colish, Mark > Wong, Brent Dombrowski, and Gabrielle Roth). Hi, thanks for the review! > - Regression test requires plpythonu; it needs to work without that. The patch contains no regression tests and -- as far as I can tell -- cannot be reliably tested in the current pg_regress framework. The plpythonu line is just an example to demonstrate the patch output. > - The doc comment 'pgstat_get_backend_current_activity' doesn't match > the function name 'pgstat_get_crashed_backend_activity'. Oops, copy-paste error. :) > - There are some formatting problems, such as all newlines at the same > indent level need to line up. (The author mentioned "not [being] > happy with the indenting depth", so I think this is not a surprise.) That was deliberate. As far as I've seen, in Postgres source, complex expressions are usually lined up to the starting parentheses, unless the expression is too long, in which case it's aligned to the 78-character right margin. I decided to use this because splitting the expression on yet one more line wouldn't improve code readability. Or have I misunderstood the coding style? > - Unknown is used a lot (see > http://developer.postgresql.org/pgdocs/postgres/error-style-guide.html#AEN94099) The string "(unknown)" in postmaster.c was there before me, I didn't change that. The other instance of "unknown" in the comment for ascii_safe_strncpy I believe expresses the function quite well -- the function doesn't know and doesn't care what the input encoding is. > We had some questions about ascii_safe_strncpy: > - It doesn't convert just unknown encodings, it converts all > encodings. Is that intentional? Technically we always "know" the right encoding -- the query is in the backend's database encoding. The point here is being simple and foolproof -- not introducing unnecessary amounts of code into the postmaster. Since ASCII characters are valid in any encoding, we only keep ASCII characters and throw away the rest. This was added in response to concerns that ereport might attempt to convert the string to another encoding and fail -- because the command string may be corrupt or because postmaster's encoding doesn't match the backend's encoding. And while this concern doesn't seem to apply with current code, it's still prudent to add it to pre-empt future changes and to protect the log file from potentially corrupt strings. See: http://archives.postgresql.org/pgsql-hackers/2011-09/msg00418.php > - Is there an existing function that already does this? The bytea->text conversion (byteaout function) does something similar when bytea_output='escape', but it doesn't preserve newline/tab characters and it doesn't currently exist as an independent function. It also uses the outdated octal representation. Regards, Marti -- 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] Inlining comparators as a performance optimisation
Peter Geoghegan wrote: > On the subject of highly ambitious optimisations to sorting, one > possibility I consider much more practicable than GPU-accelerated > sorting is simple threading; quicksort can be parallelised very > effectively, due to its divide-and-conquer nature. If we could agree > on a threading abstraction more sophisticated than forking, it's > something I'd be interested in looking at. To do so would obviously > entail lots of discussion about how that relates to whatever way we > eventually decide on implementing parallel query, and that's obviously > a difficult discussion. I agree that the next big challenge for Postgres is parallel operations. With the number of cores increasing, and with increased memory and SSD, parallel operation is even more important. Rather than parallelizing the entire backend, I imagine adding threading or helper processes for things like sorts, index scans, executor nodes, and stored procedure languages. I expect final code to be 2-3 years in the future. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Bug with pg_ctl -w/wait and config-only directories
Bruce Momjian writes: > Tom Lane wrote: >> As of fairly recently, the Fedora package also uses pg_ctl for both >> starting and stopping. We've fixed all the reasons that formerly >> existed to avoid use of pg_ctl, and it's a real PITA to try to >> implement the waiting logic at shell level. > OK, that's a good use-case to warrant barrelling ahead with improving > pg_ctl for config-only installs. Did I say that? The Fedora packages don't support config-only arrangements (if you tried, SELinux would likely deny access to whatever directory you chose ...) > What releases do we want to apply that > patch to allow postgres to dump config values and have pg_ctl use them? > This patch is required for old/new installs for pg_upgrade to work. I still think this is a matter for HEAD only. We haven't supported these cases in back branches and so there is little argument for back-patching. 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] Bug with pg_ctl -w/wait and config-only directories
Tom Lane wrote: > Bruce Momjian writes: > > Greg Stark wrote: > >> An interactive tool can dwim automatically but that isn't appropriate > >> for a startup script. A startupt script should always do the same > >> thing exactly and do that based on the OS policy, not based on > >> inspecting what programs are actually running on the machine. > > > I agree, except the Gentoo script does exactly that --- wait for > > completion using pg_ctl -w. > > As of fairly recently, the Fedora package also uses pg_ctl for both > starting and stopping. We've fixed all the reasons that formerly > existed to avoid use of pg_ctl, and it's a real PITA to try to > implement the waiting logic at shell level. OK, that's a good use-case to warrant barrelling ahead with improving pg_ctl for config-only installs. What releases do we want to apply that patch to allow postgres to dump config values and have pg_ctl use them? This patch is required for old/new installs for pg_upgrade to work. Once we decide that, I will work on the pg_upgrade code to use this as well. pg_upgrade will use the new pg_ctl but it also needs to find the data directory via the postgres binary. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Log crashed backend's query v2
This review was compiled from a PDXPUG group review (Dan Colish, Mark Wong, Brent Dombrowski, and Gabrielle Roth). -- We all agree this is a really useful feature. The patch applies cleanly to the current git master with git apply, it's in context diff, and does what it's supposed to do on Ubuntu, OSX, and gentoo. We found a few problems with it, and we'd like to see the patch resubmitted with the following changes: Tests: - Regression test requires plpythonu; it needs to work without that. Docs: - The doc comment 'pgstat_get_backend_current_activity' doesn't match the function name 'pgstat_get_crashed_backend_activity'. Project coding guidelines: - There are some formatting problems, such as all newlines at the same indent level need to line up. (The author mentioned "not [being] happy with the indenting depth", so I think this is not a surprise.) - Wayward tabs, line 2725 in pgstat.c specifically - Unknown is used a lot (see http://developer.postgresql.org/pgdocs/postgres/error-style-guide.html#AEN94099) We had some questions about ascii_safe_strncpy: - It doesn't convert just unknown encodings, it converts all encodings. Is that intentional? - Is there an existing function that already does this? Looking forward to the next revision! gabrielle (on behalf of PDXPUG) -- 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] Unremovable tuple monitoring (was: Addition of some trivial auto vacuum logging)
On 04/10/2011, at 11:26 PM, Robert Haas wrote: > On Mon, Oct 3, 2011 at 9:17 AM, Royce Ausburn wrote: >> - I'm not sure if I'm supposed to update CATALOG_VERSION_NO in catalog.h. >> In this patch I have. > > Generally that is left to the committer, as the correct value depends > on the value at the time of commit, not the time you submit the patch; > and including it in the patch tends to result in failing hunks, since > the value changes fairly frequently. >> - The VACUUM FULL implementation in cluster.c doesn't do any stats updating >> similar to vacuumlazy.c, so I haven't don't anything there… (is this right? >> A vacuum full may also encounter unremovable tuples, right?) > > We've occasionally heard grumblings about making cluster do more stats > updating, but your patch should just go along with whatever's being > done now in similar cases. I think I get this stats stuff now. Unless someone here thinks it's too hard for a new postgres dev's 2nd patch, I could take a stab. I might take a look at it tonight to get a feel for how hard, and what stats we could collect. I'll start a new thread for discussion. Attached is v2 of my patch that doesn't update CATALOG_VERSION_NO. I've also fixed the name of an argument to pgstat_report_vacuum which I don't think was particularly good, and I've replace the word "tuple" with "row" in some docs I added for consistency. I'm not sure what my next step should be. I've added this patch to the open commit fest -- is that all for now until the commit fest begins review? --Royce diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index a19e3f0..8692580 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -328,7 +328,8 @@ postgres: user database host FULL vacuumed manually, the last time it was vacuumed by the autovacuum daemon, the last time it was analyzed manually, @@ -764,6 +765,14 @@ postgres: user database host + + + pg_stat_get_unremovable_tuples(oid) + bigint + + Number of dead rows not removed in the table's last vacuum + + pg_stat_get_blocks_fetched(oid) diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 2253ca8..9c18dc7 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -353,6 +353,7 @@ CREATE VIEW pg_stat_all_tables AS pg_stat_get_tuples_hot_updated(C.oid) AS n_tup_hot_upd, pg_stat_get_live_tuples(C.oid) AS n_live_tup, pg_stat_get_dead_tuples(C.oid) AS n_dead_tup, +pg_stat_get_unremovable_tuples(C.oid) AS n_unremovable_tup, pg_stat_get_last_vacuum_time(C.oid) as last_vacuum, pg_stat_get_last_autovacuum_time(C.oid) as last_autovacuum, pg_stat_get_last_analyze_time(C.oid) as last_analyze, diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index cf8337b..140fe92 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -91,6 +91,7 @@ typedef struct LVRelStats double scanned_tuples; /* counts only tuples on scanned pages */ double old_rel_tuples; /* previous value of pg_class.reltuples */ double new_rel_tuples; /* new estimated total # of tuples */ + double unremovable_tuples; /* count of dead tuples not yet removable */ BlockNumber pages_removed; double tuples_deleted; BlockNumber nonempty_pages; /* actually, last nonempty page + 1 */ @@ -245,7 +246,8 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt, /* report results to the stats collector, too */ pgstat_report_vacuum(RelationGetRelid(onerel), onerel->rd_rel->relisshared, -new_rel_tuples); +new_rel_tuples, + vacrelstats->unremovable_tuples); /* and log the action if appropriate */ if (IsAutoVacuumWorkerProcess() && Log_autovacuum_min_duration >= 0) @@ -829,6 +831,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats, /* save stats for use later */ vacrelstats->scanned_tuples = num_tuples; vacrelstats->tuples_deleted = tups_vacuumed; + vacrelstats->unremovable_tuples = nkeep; /* now we can compute the new value for pg_class.reltuples */ vacrelstats->new_rel_tuples = vac_estimate_reltuples(onerel, false, diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index 9132db7..d974a96 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -1251,7 +1251,7 @@ pgstat_report_autovac(Oid dboid) * - */ void -pgstat_report_vacuum(Oid tableoid, bool shared, PgStat_Counter tup
Re: [HACKERS] Bug with pg_ctl -w/wait and config-only directories
Bruce Momjian writes: > Greg Stark wrote: >> An interactive tool can dwim automatically but that isn't appropriate >> for a startup script. A startupt script should always do the same >> thing exactly and do that based on the OS policy, not based on >> inspecting what programs are actually running on the machine. > I agree, except the Gentoo script does exactly that --- wait for > completion using pg_ctl -w. As of fairly recently, the Fedora package also uses pg_ctl for both starting and stopping. We've fixed all the reasons that formerly existed to avoid use of pg_ctl, and it's a real PITA to try to implement the waiting logic at shell level. 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] Bug with pg_ctl -w/wait and config-only directories
Greg Stark wrote: > On Tue, Oct 4, 2011 at 2:42 PM, Bruce Momjian wrote: > > Because pg_ctl 9.1 will read postmaster.pid and find the port number, > > socket location, and listen host for wait mode --- I doubt someone would > > do that work in a script. > > But this is the whole difference between them. An init.d script > *shouldn't* do all that. It *knows* how the system daemon is > configured and should only be used to start and stop that process. And > it can't wait, it's not an interactive tool, it has to implement the > standard init.d interface. > > An interactive tool can dwim automatically but that isn't appropriate > for a startup script. A startupt script should always do the same > thing exactly and do that based on the OS policy, not based on > inspecting what programs are actually running on the machine. I agree, except the Gentoo script does exactly that --- wait for completion using pg_ctl -w. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Does pg_settings.sourcefile/sourceline work on Windows?
Dave Page writes: > On Tue, Oct 4, 2011 at 8:27 PM, Tom Lane wrote: >> OK. I can fix that while I'm busy hacking on guc.c. Does anyone care >> enough about this to think it should be back-patched? > I think it's worthwhile if the patch can be applied fairly easily to > the older branches. If not, I don't think it's worth worrying about. It's a pretty trivial patch. Will do. 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] Does pg_settings.sourcefile/sourceline work on Windows?
On Tue, Oct 4, 2011 at 8:27 PM, Tom Lane wrote: > Dave Page writes: >> On Tue, Oct 4, 2011 at 7:55 PM, Tom Lane wrote: >>> I'm betting not, because I don't see any support for copying their >>> values down to child processes in >>> write_nondefault_variables/read_nondefault_variables. > >> Correct, it does not. > > OK. I can fix that while I'm busy hacking on guc.c. Does anyone care > enough about this to think it should be back-patched? I think it's worthwhile if the patch can be applied fairly easily to the older branches. If not, I don't think it's worth worrying about. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: 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] Does pg_settings.sourcefile/sourceline work on Windows?
Dave Page writes: > On Tue, Oct 4, 2011 at 7:55 PM, Tom Lane wrote: >> I'm betting not, because I don't see any support for copying their >> values down to child processes in >> write_nondefault_variables/read_nondefault_variables. > Correct, it does not. OK. I can fix that while I'm busy hacking on guc.c. Does anyone care enough about this to think it should be back-patched? 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] Does pg_settings.sourcefile/sourceline work on Windows?
On Tue, Oct 4, 2011 at 7:55 PM, Tom Lane wrote: > I'm betting not, because I don't see any support for copying their > values down to child processes in > write_nondefault_variables/read_nondefault_variables. Correct, it does not. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Does pg_settings.sourcefile/sourceline work on Windows?
I'm betting not, because I don't see any support for copying their values down to child processes in write_nondefault_variables/read_nondefault_variables. 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] Bug with pg_ctl -w/wait and config-only directories
On Mon, Oct 03, 2011 at 04:49:21PM -0300, Alvaro Herrera wrote: > > Excerpts from Bruce Momjian's message of lun oct 03 16:09:08 -0300 2011: > > > Yes, auto-creation of symlinks would be useful, but at that point pg_ctl > > and pg_upgrade would have to use the real data directory, so I again > > wonder what the config-only directory is getting us. > > Not mixing config stuff (in /etc per FHS) with server data (/var/lib, > again per FHS). It's Debian policy anyway. I don't judge whether this > is sane or not. See > http://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard > > > Why were people not using pg_ctl? Because of the limitations which were > > fixed in PG 9.1? As Dave already said, windows already has to use pg_ctl. > > As I said, Debian has their own version pg_ctlcluster because of their > work to allow multiple major versions to work simultaneously in the same > server. I dunno what about Gentoo. I implemented separated configuration and data directories to adhere to FHS. Actually, I had a bug on bugs.gentoo.org -- again, actually, there have been a few bugs over the years -- requesting that I make PostgreSQL adhere to the FHS. I made it work using pg_ctl. The more curious among you can take a look at the initscripts and related config files from my devspace. [1] '/etc/init.d/postgresql-9.0 restart' will actually call stop() and start(). Otherwise, I've mostly paralleled initscript functionality with the functionality of pg_ctl. Multiple initscripts are installed side-by-side to control multiple major versions. 1. http://dev.gentoo.org/~titanofold/ -- Mr. Aaron W. Swenson Gentoo Linux Developer Email: titanof...@gentoo.org GnuPG FP : 2C00 7719 4F85 FB07 A49C 0E31 5713 AA03 D1BB FDA0 GnuPG ID : D1BBFDA0 pgpv5M1wDmlL1.pgp Description: PGP signature
Re: [HACKERS] Bug with pg_ctl -w/wait and config-only directories
On Tue, Oct 04, 2011 at 09:42:42AM -0400, Bruce Momjian wrote: > Peter Eisentraut wrote: > > On m?n, 2011-10-03 at 18:44 -0400, Bruce Momjian wrote: > > > Agreed. You could argue that pg_ctl 9.1 is much better than anything > > > anyone would be able to craft in a script. > > > > And what facts support that argument? > > Because pg_ctl 9.1 will read postmaster.pid and find the port number, > socket location, and listen host for wait mode --- I doubt someone would > do that work in a script. I don't know why we chose pg_ctl, but I chose to stick with pg_ctl over rolling my own precisely because of that. pg_ctl does all sorts of fancy things that would have taken me a much longer time than figuring out a way to allow a configuration-only directory and a data-only directory. -- Mr. Aaron W. Swenson Gentoo Linux Developer Email: titanof...@gentoo.org GnuPG FP : 2C00 7719 4F85 FB07 A49C 0E31 5713 AA03 D1BB FDA0 GnuPG ID : D1BBFDA0 pgpVNrf0vvvgS.pgp Description: PGP signature
Re: [HACKERS] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.
On Tue, Oct 4, 2011 at 03:09, Amit Khandekar
wrote:
> On 4 October 2011 14:04, Alex Hunsaker wrote:
>> On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
>> wrote:
>>
>>> WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
>>> utf8_str, so pg_verify_mbstr_len() will not get called. [...]
>>
>> Consider a latin1 database where utf8_str was a string of ascii
>> characters. [...]
>> [Patch] Look ok to you?
>>
>
> + if(GetDatabaseEncoding() == PG_UTF8)
> + pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
>
> In your patch, the above will again skip mb-validation if the database
> encoding is SQL_ASCII. Note that in pg_do_encoding_conversion returns
> the un-converted string even if *one* of the src and dest encodings is
> SQL_ASCII.
*scratches head* I thought the point of SQL_ASCII was no encoding
conversion was done and so there would be nothing to verify.
Ahh I see looks like pg_verify_mbstr_len() will make sure there are no
NULL bytes in the string when we are a single byte encoding.
> I think :
> if (ret == utf8_str)
> + {
> + pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
> ret = pstrdup(ret);
> + }
>
> This (ret == utf8_str) condition would be a reliable way for knowing
> whether pg_do_encoding_conversion() has done the conversion at all.
Yes. However (and maybe im nitpicking here), I dont see any reason to
verify certain strings twice if we can avoid it.
What do you think about:
+/*
+* when we are a PG_UTF8 or SQL_ASCII database pg_do_encoding_conversion()
+* will not do any conversion or verification. we need to do it
manually instead.
+*/
+ if( GetDatabaseEncoding() == PG_UTF8 ||
GetDatabaseEncoding() == SQL_ASCII)
+ pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
--
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] have SLRU truncation use callbacks
Excerpts from Kevin Grittner's message of jue sep 29 14:51:38 -0300 2011: > Alvaro Herrera wrote: > > > But I think these changes stand on their own, merely on code > > clarity grounds). > > After a quick scan, I think it helps with that. This was a messy > area to deal with in SSI given the old API; with this change I think > we could make that part of the SSI code clearer and maybe clean up > unneeded files in a couple places where cleanup under the old API > was judged not to be worth the code complexity needed to make it > happen. Thanks for the review. I just pushed this. Note I didn't touch the predicate.c stuff at all -- I leave that to you, if you're so inclined :-) -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] Should we get rid of custom_variable_classes altogether?
Tom Lane writes: > I still don't see the point. If they want to change the default > setting, they add an entry to postgresql.conf. Problem solved. As you wish. They will have to figure the current defaults in some other way then edit the file. That's good enough for now anyway. >> We could have some extension.conf file. Appending to postgresql.conf is >> not possible from a third-party package per debian's policy, so having >> extension/foo.conf instead would make sense here. > > This is adding even more complication to solve a non-problem. Mmm. Ok. > May I remind you that a lot of people think that the default entries in > postgresql.conf for the core settings are a bad idea? Why should we > invent even more mechanism (and more conventions for users to remember) > to duplicate something of questionable value? It could be the timing when I try to sell my idea of one file per GUC, then extensions would simply add a bunch of files in there. The value of doing one GUC per file is not having to parse anything, the first non line is the value, the rest of the file free form comments. With this model, there's no new setup mechanism. But anyhow, all that can wait until after you get rid of custom_variable_classes, I think we're talking about what could happen next here, if anything. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- 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] Should we get rid of custom_variable_classes altogether?
Dimitri Fontaine writes: > Tom Lane writes: >> Dimitri Fontaine writes: >>> What I have in mind for extensions now that c_v_c is out would be to be >>> able to declare any GUC in the control file, with default values, and >>> without forcing extension to handle the GUC in its .so â I don't think >>> we have to change the code beside removing the c_v_c checks here. >> What's the point of that? A value in an extension control file isn't >> particularly easily accessible. You'd basically only see it when >> loading the extension, and that's a scenario in which the existing >> mechanism works just fine. I see no reason to break existing code >> here. > It's not about the code behavior but user support and packaging. That > the code does the right DefineCustom calls is very good, but users > should be able to easily alter defaults after installing an extension. > And you're right, putting the setup into the control file is not > providing that. I still don't see the point. If they want to change the default setting, they add an entry to postgresql.conf. Problem solved. > We could have some extension.conf file. Appending to postgresql.conf is > not possible from a third-party package per debian's policy, so having > extension/foo.conf instead would make sense here. This is adding even more complication to solve a non-problem. May I remind you that a lot of people think that the default entries in postgresql.conf for the core settings are a bad idea? Why should we invent even more mechanism (and more conventions for users to remember) to duplicate something of questionable value? 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_upgrade if 'postgres' database is dropped
pg_upgrade doesn't work if the 'postgres' database has been dropped in the old cluster: ~/pgsql.master$ bin/pg_upgrade -b ~/pgsql.91stable/bin -B bin/ -d ~/pgsql.91stable/data -D data-upgraded/ Performing Consistency Checks - Checking current, bin, and data directories ok Checking cluster versions ok Checking database user is a superuser ok Checking for prepared transactions ok Checking for reg* system OID user data typesok Checking for contrib/isn with bigint-passing mismatch ok Creating catalog dump ok Checking for prepared transactions ok New cluster database "postgres" does not exist in the old cluster Failure, exiting That's a bit unfortunate. We have some other tools that don't work without the 'postgres' database, like 'reindexdb -all', but it would still be nice if pg_upgrade did. -- Heikki Linnakangas 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] Bug with pg_ctl -w/wait and config-only directories
. Alvaro Herrera-7 wrote: > > I dunno what about Gentoo. > - some info about gentoo http://pastebin.com/9hbLmVJA http://www.gentoo.org/doc/en/postgres-howto.xml -- View this message in context: http://postgresql.1045698.n5.nabble.com/Bug-with-pg-ctl-w-wait-and-config-only-directories-tp4860202p4868931.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.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] Should we get rid of custom_variable_classes altogether?
Tom Lane writes: > Dimitri Fontaine writes: >> What I have in mind for extensions now that c_v_c is out would be to be >> able to declare any GUC in the control file, with default values, and >> without forcing extension to handle the GUC in its .so — I don't think >> we have to change the code beside removing the c_v_c checks here. > > What's the point of that? A value in an extension control file isn't > particularly easily accessible. You'd basically only see it when > loading the extension, and that's a scenario in which the existing > mechanism works just fine. I see no reason to break existing code > here. It's not about the code behavior but user support and packaging. That the code does the right DefineCustom calls is very good, but users should be able to easily alter defaults after installing an extension. And you're right, putting the setup into the control file is not providing that. We could have some extension.conf file. Appending to postgresql.conf is not possible from a third-party package per debian's policy, so having extension/foo.conf instead would make sense here. But at this point, it's nothing you need to care right now in your patch I guess, unless you're motivated enough :) Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- 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] Bug with pg_ctl -w/wait and config-only directories
Robert Haas wrote: > On Tue, Oct 4, 2011 at 10:55 AM, Bruce Momjian wrote: > >> It seems both ugly and unnecessary to declare dump_config_variable as > >> char[MAXPGPATH]. ?MAXPGPATH doesn't seem like the right length for a > >> buffer intended to hold a GUC name, but in fact I don't think you need > >> a buffer at all. ?I think you can just declare it as char * and say > >> dump_config_variable = optarg. getopt() doesn't overwrite the input > >> argument vector, does it? > > > > Well, as I remember, it writes a null byte at the end of the argument > > and then passes the pointer to the start --- when it advances to the > > next argument, it removes the null, so the pointer might still be valid, > > but does not have proper termination (or maybe that's what strtok() > > does). ?However, I can find no documentation that mentions this > > restriction, so perhaps it is old and no longer valid. > > > > If you look in our code you will see tons of cases of: > > > > ? ? ? ?user = strdup(optarg); > > ? ? ? ?pg_data = xstrdup(optarg); > > ? ? ? ?my_opts->dbname = mystrdup(optarg); > > > > However, I see other cases where we just assign optarg and optarg is a > > string, e.g. pg_dump: > > > > ? ? ? ?username = optarg; > > > > Doing a Google search shows both types of coding in random code pieces. > > > > Are the optarg string duplication calls unnecessary and can be removed? > > Either that, or we need to add some. > > Well, if you want to keep it, I'd suggest using malloc() to get an > appropriate size buffer (not palloc) rather than using some arbitrary > constant for the length. The new code does strdup(), which will match what is passed. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Bug with pg_ctl -w/wait and config-only directories
On Tue, Oct 4, 2011 at 10:55 AM, Bruce Momjian wrote: >> It seems both ugly and unnecessary to declare dump_config_variable as >> char[MAXPGPATH]. MAXPGPATH doesn't seem like the right length for a >> buffer intended to hold a GUC name, but in fact I don't think you need >> a buffer at all. I think you can just declare it as char * and say >> dump_config_variable = optarg. getopt() doesn't overwrite the input >> argument vector, does it? > > Well, as I remember, it writes a null byte at the end of the argument > and then passes the pointer to the start --- when it advances to the > next argument, it removes the null, so the pointer might still be valid, > but does not have proper termination (or maybe that's what strtok() > does). However, I can find no documentation that mentions this > restriction, so perhaps it is old and no longer valid. > > If you look in our code you will see tons of cases of: > > user = strdup(optarg); > pg_data = xstrdup(optarg); > my_opts->dbname = mystrdup(optarg); > > However, I see other cases where we just assign optarg and optarg is a > string, e.g. pg_dump: > > username = optarg; > > Doing a Google search shows both types of coding in random code pieces. > > Are the optarg string duplication calls unnecessary and can be removed? > Either that, or we need to add some. Well, if you want to keep it, I'd suggest using malloc() to get an appropriate size buffer (not palloc) rather than using some arbitrary constant for the length. -- 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] Bug with pg_ctl -w/wait and config-only directories
Robert Haas wrote:
> On Mon, Oct 3, 2011 at 11:04 PM, Bruce Momjian wrote:
> > OK, here is a patch that adds a -C option to the postmaster so any
> > config variable can be dumped, even while the server is running (there
> > is no security check because we don't have a user name at this point),
> > e.g.:
> >
> > ? ? ? ?postgres -D /pg_upgrade/tmp -C data_directory
> > ? ? ? ?/u/pg/data
>
> It seems both ugly and unnecessary to declare dump_config_variable as
> char[MAXPGPATH]. MAXPGPATH doesn't seem like the right length for a
> buffer intended to hold a GUC name, but in fact I don't think you need
> a buffer at all. I think you can just declare it as char * and say
> dump_config_variable = optarg. getopt() doesn't overwrite the input
> argument vector, does it?
Well, as I remember, it writes a null byte at the end of the argument
and then passes the pointer to the start --- when it advances to the
next argument, it removes the null, so the pointer might still be valid,
but does not have proper termination (or maybe that's what strtok()
does). However, I can find no documentation that mentions this
restriction, so perhaps it is old and no longer valid.
If you look in our code you will see tons of cases of:
user = strdup(optarg);
pg_data = xstrdup(optarg);
my_opts->dbname = mystrdup(optarg);
However, I see other cases where we just assign optarg and optarg is a
string, e.g. pg_dump:
username = optarg;
Doing a Google search shows both types of coding in random code pieces.
Are the optarg string duplication calls unnecessary and can be removed?
Either that, or we need to add some.
> Also, I think you should remove this comment:
>
> + /* This allows anyone to read super-user config values. */
>
> It allows anyone to read super-user config values *who can already
> read postgresql.conf*. Duh.
Oh, very good point --- I had not realized that. Comment updated.
> > It also modifies pg_ctl to use this feature. ?It works fine for pg_ctl
> > -w start/stop with a config-only directory, so this is certainly in the
> > right direction. ?You can also use pg_ctl -o '--data_directory=/abc' and
> > it will be understood:
> >
> > ? ? ? ?pg_ctl -o '--data_directory=/u/pg/data' -D tmp start
> >
> > If you used --data_directory to start the server, you will need to use
> > --data_directory to stop it, which seems reasonable.
>
> Yep. I think that when adjust_data_dir() changes pg_data it probably
> needs to call canonicalize_path() on the new value.
Done.
> > Patch attached. ?This was much simpler than I thought. ?:-)
>
> Yes, this looks pretty simple.
What really saved my bacon was the fact that the -o parameters are
passed into the backend and processed by the backend, rather than pg_ctl
having to read through that mess and parse it. Doing that logic was
going to be very hard and unlikely to be back-patch-able.
Updated patch attached.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
new file mode 100644
index 0a84d97..122c206
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*** bool enable_bonjour = false;
*** 203,208
--- 203,210
char *bonjour_name;
bool restart_after_crash = true;
+ char *output_config_variable = NULL;
+
/* PIDs of special child processes; 0 when not running */
static pid_t StartupPID = 0,
BgWriterPID = 0,
*** PostmasterMain(int argc, char *argv[])
*** 537,543
* tcop/postgres.c (the option sets should not conflict) and with the
* common help() function in main/main.c.
*/
! while ((opt = getopt(argc, argv, "A:B:bc:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:W:-:")) != -1)
{
switch (opt)
{
--- 539,545
* tcop/postgres.c (the option sets should not conflict) and with the
* common help() function in main/main.c.
*/
! while ((opt = getopt(argc, argv, "A:B:bc:C:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:W:-:")) != -1)
{
switch (opt)
{
*** PostmasterMain(int argc, char *argv[])
*** 554,559
--- 556,565
IsBinaryUpgrade = true;
break;
+ case 'C':
+ output_config_variable = strdup(optarg);
+ break;
+
case 'D':
userDoption = optarg;
break;
*** PostmasterMain(int argc, char *argv[])
*** 728,733
--- 734,746
if (!SelectConfigFiles(userDoption, progname))
ExitPostmaster(2);
+ if (output_config_variable != NULL)
+ {
+ /* permission is handled because the user is reading inside the data dir */
+ puts(GetConfigOption(output_config_variable, false, false));
+ ExitPostmaster(0);
+ }
+
/* Verify that DataDir looks reasonable */
checkDataDir();
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
new fi
Re: [HACKERS] Should we get rid of custom_variable_classes altogether?
Dimitri Fontaine writes: > What I have in mind for extensions now that c_v_c is out would be to be > able to declare any GUC in the control file, with default values, and > without forcing extension to handle the GUC in its .so â I don't think > we have to change the code beside removing the c_v_c checks here. What's the point of that? A value in an extension control file isn't particularly easily accessible. You'd basically only see it when loading the extension, and that's a scenario in which the existing mechanism works just fine. I see no reason to break existing code here. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug with pg_ctl -w/wait and config-only directories
On Tue, Oct 4, 2011 at 2:42 PM, Bruce Momjian wrote: > Because pg_ctl 9.1 will read postmaster.pid and find the port number, > socket location, and listen host for wait mode --- I doubt someone would > do that work in a script. But this is the whole difference between them. An init.d script *shouldn't* do all that. It *knows* how the system daemon is configured and should only be used to start and stop that process. And it can't wait, it's not an interactive tool, it has to implement the standard init.d interface. An interactive tool can dwim automatically but that isn't appropriate for a startup script. A startupt script should always do the same thing exactly and do that based on the OS policy, not based on inspecting what programs are actually running on the machine. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug with pg_ctl -w/wait and config-only directories
Peter Eisentraut wrote: > On m?n, 2011-10-03 at 18:44 -0400, Bruce Momjian wrote: > > Agreed. You could argue that pg_ctl 9.1 is much better than anything > > anyone would be able to craft in a script. > > And what facts support that argument? Because pg_ctl 9.1 will read postmaster.pid and find the port number, socket location, and listen host for wait mode --- I doubt someone would do that work in a script. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] pg_last_xact_insert_timestamp
On Tue, Oct 4, 2011 at 1:05 PM, Fujii Masao wrote: > On Tue, Oct 4, 2011 at 5:33 AM, Robert Haas wrote: >> On Mon, Oct 3, 2011 at 4:25 PM, Simon Riggs wrote: >>> On Mon, Oct 3, 2011 at 8:07 PM, Robert Haas wrote: >>> Sorry, but I still don't really think it's fair to say that you've proposed a solution to this problem. Or if you have, neither I nor Fujii Masao understand that proposal well enough to decide whether we like it. >>> >>> Arguing between trenches doesn't really get us anywhere. >>> >>> As ever, when someone claims to have a better solution then it is up >>> to them to prove that is the case. >> >> So... are you going to do that? > > Simon, could you? If your proposal turns out to be better than mine, I'd be > happy to agree to drop my patch and adopt yours. Yes, will do. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP statement reworks
On Wed, Sep 28, 2011 at 11:51 AM, Kohei KaiGai wrote: > I rebased the patches towards the latest git master, so I believe these > are available to apply. Reviewing away... I don't see why we need one struct called ObjectProperty and another called CatalogProperty. Just fold CatalogProperty into ObjectProperty and make it one big flat structure. The get_object_property_waddawadda functions appear to be designed under the assumption that each object type will be in the ObjectProperty structure at the offset corresponding to (int) objtype. Are these functions going to get called from any place that's performance-critical enough to justify that optimization? I'd rather just have these functions use linear search, so that if someone adds a new OBJECT_* constant and doesn't add it to the array it doesn't immediately break everything. get_object_namespace() looks like it ought to live in objectaddress.c, not dropcmds.c. check_object_validation() doesn't seem like a very good description of what that code actually does -- and that code looks like it's copy-and-pasted from elsewhere. Can we avoid that? get_relation_address() is surely a copy of some other bit of code; how can we avoid duplication? Setting status to "Waiting on Author". -- 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] [PATCH] Unremovable tuple monitoring (was: Addition of some trivial auto vacuum logging)
On Mon, Oct 3, 2011 at 9:17 AM, Royce Ausburn wrote: > - I'm not sure if I'm supposed to update CATALOG_VERSION_NO in catalog.h. In > this patch I have. Generally that is left to the committer, as the correct value depends on the value at the time of commit, not the time you submit the patch; and including it in the patch tends to result in failing hunks, since the value changes fairly frequently. > - I'm not sure about how I should be selecting an OID for my new stats > function. I used the unused_oids script and picked one that seemed > reasonable. That's the way to do it. > - The VACUUM FULL implementation in cluster.c doesn't do any stats updating > similar to vacuumlazy.c, so I haven't don't anything there… (is this right? > A vacuum full may also encounter unremovable tuples, right?) We've occasionally heard grumblings about making cluster do more stats updating, but your patch should just go along with whatever's being done now in similar cases. -- 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] Bug with pg_ctl -w/wait and config-only directories
On Mon, Oct 3, 2011 at 11:04 PM, Bruce Momjian wrote: > OK, here is a patch that adds a -C option to the postmaster so any > config variable can be dumped, even while the server is running (there > is no security check because we don't have a user name at this point), > e.g.: > > postgres -D /pg_upgrade/tmp -C data_directory > /u/pg/data It seems both ugly and unnecessary to declare dump_config_variable as char[MAXPGPATH]. MAXPGPATH doesn't seem like the right length for a buffer intended to hold a GUC name, but in fact I don't think you need a buffer at all. I think you can just declare it as char * and say dump_config_variable = optarg. getopt() doesn't overwrite the input argument vector, does it? Also, I think you should remove this comment: + /* This allows anyone to read super-user config values. */ It allows anyone to read super-user config values *who can already read postgresql.conf*. Duh. > It also modifies pg_ctl to use this feature. It works fine for pg_ctl > -w start/stop with a config-only directory, so this is certainly in the > right direction. You can also use pg_ctl -o '--data_directory=/abc' and > it will be understood: > > pg_ctl -o '--data_directory=/u/pg/data' -D tmp start > > If you used --data_directory to start the server, you will need to use > --data_directory to stop it, which seems reasonable. Yep. I think that when adjust_data_dir() changes pg_data it probably needs to call canonicalize_path() on the new value. > Patch attached. This was much simpler than I thought. :-) Yes, this looks pretty simple. -- 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] Double sorting split patch
On Tue, Oct 4, 2011 at 1:46 PM, Heikki Linnakangas <
heikki.linnakan...@enterprisedb.com> wrote:
> Ok. Could you phrase that as a code comment?
>
> Here's a version of the patch I've been working on. There's no functional
> changes, just a lot of moving things around, comment changes, etc. to
> hopefully make it more readable.
Thanks for your work on this patch. Patch with comment is attached.
--
With best regards,
Alexander Korotkov.
*** a/src/backend/access/gist/gistproc.c
--- b/src/backend/access/gist/gistproc.c
***
*** 26,31 static bool gist_box_leaf_consistent(BOX *key, BOX *query,
--- 26,35
static double size_box(Datum dbox);
static bool rtree_internal_consistent(BOX *key, BOX *query,
StrategyNumber strategy);
+ static BOX *empty_box(void);
+
+ /* Minimal possible ratio of split */
+ #define LIMIT_RATIO 0.3
/**
***
*** 49,78 rt_box_union(PG_FUNCTION_ARGS)
PG_RETURN_BOX_P(n);
}
- static Datum
- rt_box_inter(PG_FUNCTION_ARGS)
- {
- BOX *a = PG_GETARG_BOX_P(0);
- BOX *b = PG_GETARG_BOX_P(1);
- BOX *n;
-
- n = (BOX *) palloc(sizeof(BOX));
-
- n->high.x = Min(a->high.x, b->high.x);
- n->high.y = Min(a->high.y, b->high.y);
- n->low.x = Max(a->low.x, b->low.x);
- n->low.y = Max(a->low.y, b->low.y);
-
- if (n->high.x < n->low.x || n->high.y < n->low.y)
- {
- pfree(n);
- /* Indicate "no intersection" by returning NULL pointer */
- n = NULL;
- }
-
- PG_RETURN_BOX_P(n);
- }
-
/*
* The GiST Consistent method for boxes
*
--- 53,58
***
*** 194,279 gist_box_penalty(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(result);
}
- static void
- chooseLR(GIST_SPLITVEC *v,
- OffsetNumber *list1, int nlist1, BOX *union1,
- OffsetNumber *list2, int nlist2, BOX *union2)
- {
- bool firstToLeft = true;
-
- if (v->spl_ldatum_exists || v->spl_rdatum_exists)
- {
- if (v->spl_ldatum_exists && v->spl_rdatum_exists)
- {
- BOX LRl = *union1,
- LRr = *union2;
- BOX RLl = *union2,
- RLr = *union1;
- double sizeLR,
- sizeRL;
-
- adjustBox(&LRl, DatumGetBoxP(v->spl_ldatum));
- adjustBox(&LRr, DatumGetBoxP(v->spl_rdatum));
- adjustBox(&RLl, DatumGetBoxP(v->spl_ldatum));
- adjustBox(&RLr, DatumGetBoxP(v->spl_rdatum));
-
- sizeLR = size_box(DirectFunctionCall2(rt_box_inter, BoxPGetDatum(&LRl), BoxPGetDatum(&LRr)));
- sizeRL = size_box(DirectFunctionCall2(rt_box_inter, BoxPGetDatum(&RLl), BoxPGetDatum(&RLr)));
-
- if (sizeLR > sizeRL)
- firstToLeft = false;
-
- }
- else
- {
- float p1,
- p2;
- GISTENTRY oldUnion,
- addon;
-
- gistentryinit(oldUnion, (v->spl_ldatum_exists) ? v->spl_ldatum : v->spl_rdatum,
- NULL, NULL, InvalidOffsetNumber, FALSE);
-
- gistentryinit(addon, BoxPGetDatum(union1), NULL, NULL, InvalidOffsetNumber, FALSE);
- DirectFunctionCall3(gist_box_penalty, PointerGetDatum(&oldUnion), PointerGetDatum(&addon), PointerGetDatum(&p1));
- gistentryinit(addon, BoxPGetDatum(union2), NULL, NULL, InvalidOffsetNumber, FALSE);
- DirectFunctionCall3(gist_box_penalty, PointerGetDatum(&oldUnion), PointerGetDatum(&addon), PointerGetDatum(&p2));
-
- if ((v->spl_ldatum_exists && p1 > p2) || (v->spl_rdatum_exists && p1 < p2))
- firstToLeft = false;
- }
- }
-
- if (firstToLeft)
- {
- v->spl_left = list1;
- v->spl_right = list2;
- v->spl_nleft = nlist1;
- v->spl_nright = nlist2;
- if (v->spl_ldatum_exists)
- adjustBox(union1, DatumGetBoxP(v->spl_ldatum));
- v->spl_ldatum = BoxPGetDatum(union1);
- if (v->spl_rdatum_exists)
- adjustBox(union2, DatumGetBoxP(v->spl_rdatum));
- v->spl_rdatum = BoxPGetDatum(union2);
- }
- else
- {
- v->spl_left = list2;
- v->spl_right = list1;
- v->spl_nleft = nlist2;
- v->spl_nright = nlist1;
- if (v->spl_ldatum_exists)
- adjustBox(union2, DatumGetBoxP(v->spl_ldatum));
- v->spl_ldatum = BoxPGetDatum(union2);
- if (v->spl_rdatum_exists)
- adjustBox(union1, DatumGetBoxP(v->spl_rdatum));
- v->spl_rdatum = BoxPGetDatum(union1);
- }
-
- v->spl_ldatum_exists = v->spl_rdatum_exists = false;
- }
-
/*
* Trivial split: half of entries will be placed on one page
* and another half - to another
--- 174,179
***
*** 338,536 fallbackSplit(GistEntryVector *entryvec, GIST_SPLITVEC *v)
}
/*
! * The GiST PickSplit method
*
! * New linear algorithm, see 'New Linear Node Splitting Algorithm for R-tree',
! * C.H.Ang and T.C.Tan
*
! * This is used for both boxes and points.
*/
Datum
gist_box_picksplit(PG_FUNCTION_ARGS)
{
GistEntryVector *entryvec = (GistEntryVector *) PG_GETARG_POINTER(0);
GIST_SPLITVEC *v = (GIST_SPLITVEC *) PG_GETARG_POINTER(1);
! OffsetNumber i;
! OffsetNumber *listL,
! *listR,
! *listB,
! *listT;
! BOX *unionL,
! *unionR,
! *unionB,
! *unionT;
!
Re: [HACKERS] [REVIEW] pg_last_xact_insert_timestamp
On Tue, Oct 4, 2011 at 5:33 AM, Robert Haas wrote: > On Mon, Oct 3, 2011 at 4:25 PM, Simon Riggs wrote: >> On Mon, Oct 3, 2011 at 8:07 PM, Robert Haas wrote: >> >>> Sorry, but I still don't really think it's fair to say that you've >>> proposed a solution to this problem. Or if you have, neither I nor >>> Fujii Masao understand that proposal well enough to decide whether we >>> like it. >> >> Arguing between trenches doesn't really get us anywhere. >> >> As ever, when someone claims to have a better solution then it is up >> to them to prove that is the case. > > So... are you going to do that? Simon, could you? If your proposal turns out to be better than mine, I'd be happy to agree to drop my patch and adopt yours. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] pg_last_xact_insert_timestamp
On Mon, Oct 3, 2011 at 6:31 PM, Fujii Masao wrote:
>> Also, in pg_last_xact_insert_timestamp, the errhint() seems a little
>> strange - this is not exactly a WAL *control* function, is it?
>
> Not only "control" but also "WAL" might be confusing. What about
> "transaction information functions"?
Attached is the updated version of the patch. In the patch, I used the
function name itself in the HINT message, i.e., the HINT message is
the following.
pg_last_xact_insert_timestamp() cannot be executed during recovery.
>> In the documentation, for the short description of
>> pg_last_xact_insert_timestamp(), how about something like "returns the
>> time at which a transaction commit or transaction about record was
>> last inserted into the transaction log"? Or maybe that's too long.
>> But the current description doesn't seem to do much other than
>> recapitulate the function name, so I'm wondering if we can do any
>> better than that.
>
> Agreed. I will change the description per your suggestion.
Done.
>> I think that instead of hacking up the backend-status copying code to
>> have a mode where it copies everything, you should just have a
>> special-purpose function that computes the value you need directly off
>> the backend status entries themselves. This approach seems like it
>> both clutters the code and adds lots of extra data copying.
>
> Agreed. Will change.
Done.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 13996,14001 SELECT set_config('log_statement_stats', 'off', false);
--- 13996,14004
pg_current_xlog_location
+ pg_last_xact_insert_timestamp
+
+
pg_start_backup
***
*** 14049,14054 SELECT set_config('log_statement_stats', 'off', false);
--- 14052,14067
+ pg_last_xact_insert_timestamp()
+
+timestamp with time zone
+
+ Get the time at which a transaction commit or transaction abort record
+ was last inserted into the transaction log
+
+
+
+
pg_start_backup(label text , fast boolean )
text
***
*** 14175,14180 postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
--- 14188,14200
+ pg_last_xact_insert_timestamp displays the time stamp of last inserted
+ transaction. This is the time at which the commit or abort WAL record for that transaction.
+ If there has been no transaction committed or aborted yet since the server has started,
+ this function returns NULL.
+
+
+
For details about proper usage of these functions, see
.
*** a/doc/src/sgml/high-availability.sgml
--- b/doc/src/sgml/high-availability.sgml
***
*** 867,872 primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
--- 867,881
ps command (see for details).
+ You can also calculate the lag in time stamp by comparing the last
+ WAL insert time stamp on the primary with the last WAL replay
+ time stamp on the standby. They can be retrieved using
+ pg_last_xact_insert_timestamp on the primary and
+ the pg_last_xact_replay_timestamp on the standby,
+ respectively (see and
+ for details).
+
+
You can retrieve a list of WAL sender processes via the
pg_stat_replication view. Large differences between
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
***
*** 1066,1071 RecordTransactionCommit(void)
--- 1066,1074
(void) XLogInsert(RM_XACT_ID, XLOG_XACT_COMMIT_COMPACT, rdata);
}
+
+ /* Save timestamp of latest transaction commit record */
+ pgstat_report_xact_end_timestamp(xactStopTimestamp);
}
/*
***
*** 1434,1439 RecordTransactionAbort(bool isSubXact)
--- 1437,1445
(void) XLogInsert(RM_XACT_ID, XLOG_XACT_ABORT, rdata);
+ /* Save timestamp of latest transaction abort record */
+ pgstat_report_xact_end_timestamp(xlrec.xact_time);
+
/*
* Report the latest async abort LSN, so that the WAL writer knows to
* flush this abort. There's nothing to be gained by delaying this, since
***
*** 4968,4970 xact_desc(StringInfo buf, uint8 xl_info, char *rec)
--- 4974,5000
else
appendStringInfo(buf, "UNKNOWN");
}
+
+ /*
+ * Returns timestamp of latest inserted commit/abort record.
+ *
+ * If there has been no transaction committed or aborted yet since
+ * the server has started, this function returns NULL.
+ */
+ Datum
+ pg_last_xact_insert_timestamp(PG_FUNCTION_ARGS)
+ {
+ TimestampTz xtime;
+
+ if (RecoveryInProgress())
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("recovery is in progres
[HACKERS] ToDo: allow to get a number of processed rows by COPY statement
Hello
There is not possible to get a number of processed rows when COPY is
evaluated via SPI. Client can use a tag, but SPI doesn't use a tag.
I propose a small change a ProcessUtility to return a processed rows.
Note: I found a small inconsistency between SPI and Utility interface.
SPI still use a 4 byte unsign int for storing a number of processed
rows. Utility use a 8bytes unsign int.
Motivation:
postgres=# \sf fx
CREATE OR REPLACE FUNCTION public.fx(tablename text, filename text)
RETURNS integer
LANGUAGE plpgsql
AS $function$
declare r int;
begin
execute format('COPY %s FROM %s', quote_ident(tablename),
quote_literal(filename));
get diagnostics r = row_count;
return r;
end;
$function$
Regards
Pavel Stehule
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 398bc40..a7c2b8f 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -600,6 +600,7 @@ postquel_getnext(execution_state *es, SQLFunctionCachePtr fcache)
es->qd->params,
false, /* not top level */
es->qd->dest,
+ NULL,
NULL);
result = true; /* never stops early */
}
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 688279c..21cabcc 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1838,6 +1838,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
{
Node *stmt = (Node *) lfirst(lc2);
bool canSetTag;
+ bool isCopyStmt = false;
DestReceiver *dest;
_SPI_current->processed = 0;
@@ -1857,6 +1858,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
{
CopyStmt *cstmt = (CopyStmt *) stmt;
+ isCopyStmt = true;
if (cstmt->filename == NULL)
{
my_res = SPI_ERROR_COPY;
@@ -1911,16 +1913,23 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
}
else
{
+uint32 processed;
+
ProcessUtility(stmt,
plansource->query_string,
paramLI,
false, /* not top level */
dest,
- NULL);
+ NULL,
+ &processed);
/* Update "processed" if stmt returned tuples */
+
if (_SPI_current->tuptable)
_SPI_current->processed = _SPI_current->tuptable->alloced -
_SPI_current->tuptable->free;
+else if (canSetTag && isCopyStmt)
+ _SPI_current->processed = processed;
+
res = SPI_OK_UTILITY;
}
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 466727b..1a861ee 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -1184,7 +1184,8 @@ PortalRunUtility(Portal portal, Node *utilityStmt, bool isTopLevel,
portal->portalParams,
isTopLevel,
dest,
- completionTag);
+ completionTag,
+ NULL);
/* Some utility statements may change context on us */
MemoryContextSwitchTo(PortalGetHeapMemory(portal));
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 0749227..35db28c 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -319,6 +319,9 @@ CheckRestrictedOperation(const char *cmdname)
* completionTag is only set nonempty if we want to return a nondefault status.
*
* completionTag may be NULL if caller doesn't want a status string.
+ *
+ * processed may be NULL if caller doesn't want a number of processed rows
+ * by COPY statement
*/
void
ProcessUtility(Node *parsetree,
@@ -326,7 +329,8 @@ ProcessUtility(Node *parsetree,
ParamListInfo params,
bool isTopLevel,
DestReceiver *dest,
- char *completionTag)
+ char *completionTag,
+ uint32 *processed)
{
Assert(queryString != NULL); /* required as of 8.4 */
@@ -337,10 +341,10 @@ ProcessUtility(Node *parsetree,
*/
if (ProcessUtility_hook)
(*ProcessUtility_hook) (parsetree, queryString, params,
-isTopLevel, dest, completionTag);
+isTopLevel, dest, completionTag, processed);
else
standard_ProcessUtility(parsetree, queryString, params,
-isTopLevel, dest, completionTag);
+isTopLevel, dest, completionTag, processed);
}
void
@@ -349,7 +353,8 @@ standard_ProcessUtility(Node *parsetree,
ParamListInfo params,
bool isTopLevel,
DestReceiver *dest,
- char *completionTag)
+ char *completionTag,
+ uint32 *processed)
{
check_xact_readonly(parsetree);
@@ -571,6 +576,7 @@ standard_ProcessUtility(Node *parsetree,
params,
false,
None_Receiver,
+ NULL,
NULL);
}
@@ -716,12 +722,14 @@ standard_ProcessUtility(Node *parsetree,
case T_CopyStmt:
{
-uint64 processed;
+uint64 _processed;
-processed = DoCopy((CopyStmt *) parsetree, queryString);
+_processed = DoCopy((CopyStmt *) parsetree, queryString);
if (completionTag)
snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
- "COPY " UINT64_FORMAT, processed);
+
Re: [HACKERS] WIP: Join push-down for foreign tables
Kaigai-san,
Thanks for the review.
(2011/10/03 17:07), Kohei KaiGai wrote:
> At first, I tried to use file_fdw, however, it was crashed of course.
> It seems to me this logic should be modified to confirm whether the target FDW
> support join push down, or not.
>
> + if (enable_foreignjoin&&
> + joinrel->serverid != InvalidOid&&
> + (IsA(outerpath, ForeignPath) || IsA(outerpath, ForeignJoinPath))&&
> + (IsA(inner_cheapest_total, ForeignPath) ||
> +IsA(inner_cheapest_total, ForeignJoinPath)))
> +
> + {
> + ForeignJoinPath*path;
> + path = create_foreignjoin_path(root,
> + joinrel,
> + jointype,
> + sjinfo,
> + outerpath,
> + inner_cheapest_total,
> + restrictlist,
> + merge_pathkeys);
> + if (path != NULL)
> + add_path(joinrel, (Path *) path);
> + }
> +
>
> In my opinion, FdwRoutine should have an additional API to inform the core its
> supported features; such as inner-join, outer-join, order-by,
> group-by, aggregate
> functions, insert, update, delete, etc... in the future version.
Sure, so in my design PlanForeignJoin is optional.
The lack of capability is informed from FDW with setting function
pointer in FdwRoutine to NULL. If PlanForeignJoin was NULL, core
(planner) will give up to consider join push-down, and use one of local
join methods such as NestLoop and MergeJoin for those foreign tables.
As you say, other push-down-able features would also have optional
handler function for each.
BTW, what is the point of separating inner-join and outer-join in
context of push-down? I think that providing the type of the join to
FDW as parameter of PlanForeignJoin is enough. Then they can tell core
planner to give up considering join push-down by returning NULL if the
type of the join was not supported.
> Obviously, it is not hard to implement inner/outer-join feature for
> pgsql_fdw module,
> but it may be a tough work for memcached_fdw module.
Agreed, join push-down might be useful for only RDBMS wrappers. NoSQL
wrapper would not provide handler functions other than required ones for
simple scan.
>> *) SELECT * FROM A JOIN B (...) doesn't work. Specifying columns in
>> SELECT clause explicitly like "SELECT A.col1, A.col2, ..." seems to work.
>> *) ORDER BY causes error if no column is specified in SELECT clause from
>> sort key's table.
>>
> I doubt these issues are in pgsql_fdw side, not the proposed patch itself.
Yes, this problem is from pgsql_fdw's SQL generator. I'll fix it.
> In the case when the table and column names/types are compatible between
> local-side and remote-side, the problem was not reproduced in my environment.
> I'd like to suggest you to add a functionality to map remote column names to
> the local ones in pgsql_fdw.
Since per-column FDW options patch has been committed in last CF, It's
not hard to implement colname FDW option for pgsql_fdw, and rough
implementation has been already done. In next post you will be able to
map column names. :)
Regards,
--
Shigeru Hanada
--
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] restoring an object to a different name
On Oct4, 2011, at 00:59 , Andrew Dunstan wrote: > However, there are lots of wrinkles. For example, the names of objects appear > in LOTS of places, and making sure we caught them all might be quite tricky. > Say you have a table x that inherits a,b, and c, and you decide to restore > with b renamed. Now x will have a dependency on b recorded, but finding b in > the opaque sql string that is stored for the creation of x is not going to be > easy (don't anyone mention regexes here - this is not a good case for their > use IMNSHO, much as I love them). > > One idea I came up with was to set up the SQL using OIDS instead of names as > placeholders, and then replacing the OIDS with the right name at run time. So > if we want to restore something with a different name, we'd just change the > stored name in the node where it's defined and the new name would then be > picked up everywhere it's used (might need a pair, > but the idea would be the same). Hm, that is pretty much what happens if you rename the object after restoring it (using ALTER ... RENAME ...), since all catalog references happen by OID not by name. The only case where renaming the object after restoring it doesn't work is if the object's original name collides with the name of an already existing object. But solving that seems much simpler than renaming objects while restoring them. We could, for example, simply rename the pre-existing object prior to restoring, and rename it back aftwards. best regards, Florian Pflug -- 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] Double sorting split patch
On 04.10.2011 11:51, Alexander Korotkov wrote:
On Tue, Oct 4, 2011 at 12:12 PM, Heikki Linnakangas<
heikki.linnakan...@enterprisedb.com> wrote:
Can you elaborate the consider-split algorithm? The criteria to select the
new split over the previously selected one is this:
! /*
!* If ratio is acceptable, we should compare current split
with
!* previously selected one. If no split was selected then
we select
!* current anyway. Between splits of one dimension we
search for
!* minimal overlap (allowing negative values) and minimal
ration
!* (between same overlaps. We switch dimension if find
less overlap
!* (non-negative) or less range with same overlap.
!*/
! range = diminfo->upper - diminfo->lower;
! overlap = ((leftUpper) - (rightLower)) / range;
! if (context->first ||
! (context->dim == dimNum&&
!(overlap< context->overlap ||
! (overlap == context->overlap&& ratio>
context->ratio))) ||
! (context->dim != dimNum&&
!((range> context->range&&
! non_negative(overlap)<=
non_negative(context->overlap)**) ||
! non_negative(overlap)<
non_negative(context->overlap)**))
! )
! {
Why are negative overlaps handled differently across dimensions and within
the same dimension? Your considerSplit algorithm in the SYRCoSE 2011 paper
doesn't seem to make that distinction.
Yes, I've changed this behaviour after experiments on real-life datasets. On
the datasets where data don't overlap themselfs (points are always such
datasets), non-overlapping splits are frequently possible. In this case
splits tends to be along one dimension, because most distant non-overlapping
splits (i.e. having lowest negative overlap) appears to be in the same
dimension as previous split. Therefore MBRs appear to be very prolonged
along another dimension, that's bad for search. In order to prevent this
behavour I've made transition to another dimension by non-nagative part of
overlap and range. Using range as the split criteria makes MBRs more
quadratic, and using non-negative part of overlap as priority criteria give
to range chance to matter.
Ok. Could you phrase that as a code comment?
Here's a version of the patch I've been working on. There's no
functional changes, just a lot of moving things around, comment changes,
etc. to hopefully make it more readable.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
diff --git a/src/backend/access/gist/gistproc.c b/src/backend/access/gist/gistproc.c
index 43c4b12..af1a42d 100644
--- a/src/backend/access/gist/gistproc.c
+++ b/src/backend/access/gist/gistproc.c
@@ -26,6 +26,10 @@ static bool gist_box_leaf_consistent(BOX *key, BOX *query,
static double size_box(Datum dbox);
static bool rtree_internal_consistent(BOX *key, BOX *query,
StrategyNumber strategy);
+static BOX *empty_box(void);
+
+/* Minimal possible ratio of split */
+#define LIMIT_RATIO 0.3
/**
@@ -49,30 +53,6 @@ rt_box_union(PG_FUNCTION_ARGS)
PG_RETURN_BOX_P(n);
}
-static Datum
-rt_box_inter(PG_FUNCTION_ARGS)
-{
- BOX *a = PG_GETARG_BOX_P(0);
- BOX *b = PG_GETARG_BOX_P(1);
- BOX *n;
-
- n = (BOX *) palloc(sizeof(BOX));
-
- n->high.x = Min(a->high.x, b->high.x);
- n->high.y = Min(a->high.y, b->high.y);
- n->low.x = Max(a->low.x, b->low.x);
- n->low.y = Max(a->low.y, b->low.y);
-
- if (n->high.x < n->low.x || n->high.y < n->low.y)
- {
- pfree(n);
- /* Indicate "no intersection" by returning NULL pointer */
- n = NULL;
- }
-
- PG_RETURN_BOX_P(n);
-}
-
/*
* The GiST Consistent method for boxes
*
@@ -194,86 +174,6 @@ gist_box_penalty(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(result);
}
-static void
-chooseLR(GIST_SPLITVEC *v,
- OffsetNumber *list1, int nlist1, BOX *union1,
- OffsetNumber *list2, int nlist2, BOX *union2)
-{
- bool firstToLeft = true;
-
- if (v->spl_ldatum_exists || v->spl_rdatum_exists)
- {
- if (v->spl_ldatum_exists && v->spl_rdatum_exists)
- {
- BOX LRl = *union1,
- LRr = *union2;
- BOX RLl = *union2,
- RLr = *union1;
- double sizeLR,
- sizeRL;
-
- adjustBox(&LRl, DatumGetBoxP(v->spl_ldatum));
- adjustBox(&LRr, DatumGetBoxP(v->spl_rdatum));
- adjustBox(&RLl, DatumGetBoxP(v->spl_ldatum));
- adjustBox(&RLr, DatumGetBoxP(v->spl_rdatum));
-
- sizeLR = size_box(DirectFunctionCall2(rt_box_inter, BoxPGetDatum(&LRl), BoxPGetDatum(&LRr)));
- sizeRL = size_box(DirectFunctionCall2(rt_box_inter, BoxPGetDatum(&RLl), BoxPGetDatum(&RLr)));
-
- if (sizeLR > sizeRL)
-firstToLeft = false;
-
- }
- else
- {
- float p1,
- p2;
- GISTENTRY oldUnion,
- addon;
-
- gistentryinit(oldUnion, (v->spl_ldatu
Re: [HACKERS] Separating bgwriter and checkpointer
On Tue, Oct 4, 2011 at 2:51 AM, Dickson S. Guedes wrote: > 2011/10/3 Simon Riggs : >> On Sun, Oct 2, 2011 at 11:45 PM, Dickson S. Guedes >> wrote: >>> I'm trying your patch, it was applied cleanly to master and compiled >>> ok. But since I started postgres I'm seeing a 99% of CPU usage: >> >> Oh, thanks. I see what happened. I was toying with the idea of going >> straight to a WaitLatch implementation for the loop but decided to >> leave it out for a later patch, and then skipped the sleep as well. >> >> New version attached. > > Working now but even passing all tests for make check, the > regress_database's postmaster doesn't shutdown properly. > > $ make check > ... > ... > == creating temporary installation == > == initializing database system == > == starting postmaster == > running on port 57432 with PID 20094 > == creating database "regression" == > ... > == shutting down postmaster == > pg_ctl: server does not shut down > pg_regress: could not stop postmaster: exit code was 256 > > $ uname -a > Linux betelgeuse 2.6.38-11-generic-pae #50-Ubuntu SMP Mon Sep 12 > 22:21:04 UTC 2011 i686 i686 i386 GNU/Linux > > $ grep "$ ./configure" config.log > $ ./configure --enable-debug --enable-cassert > --prefix=/srv/postgres/bgwriter_split Yes, I see this also. At the same time, pg_ctl start and stop seem to work fine in every mode, which is what I tested. Which seems a little weird. I seem to be having problems with HEAD as well. Investigating further. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.
On 4 October 2011 14:04, Alex Hunsaker wrote:
> On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
> wrote:
>
>> WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
>> utf8_str, so pg_verify_mbstr_len() will not get called. That's the
>> reason, pg_verify_mbstr_len() is under the ( ret == utf8_str )
>> condition.
>
> Consider a latin1 database where utf8_str was a string of ascii
> characters. Then no conversion would take place and ret == utf8_str
> but the string would be verified by pg_do_encdoing_conversion() and
> verified again by your added check :-).
>
>>> It might be worth adding a regression test also...
>>
>> I could not find any basic pl/perl tests in the regression
>> serial_schedule. I am not sure if we want to add just this scenario
>> without any basic tests for pl/perl ?
>
> I went ahead and added one in the attached based upon your example.
>
> Look ok to you?
>
+ if(GetDatabaseEncoding() == PG_UTF8)
+ pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
In your patch, the above will again skip mb-validation if the database
encoding is SQL_ASCII. Note that in pg_do_encoding_conversion returns
the un-converted string even if *one* of the src and dest encodings is
SQL_ASCII.
I think :
if (ret == utf8_str)
+ {
+ pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
ret = pstrdup(ret);
+ }
This (ret == utf8_str) condition would be a reliable way for knowing
whether pg_do_encoding_conversion() has done the conversion at all.
I am ok with the new test. Thanks for doing it yourself !
> BTW thanks for the patch!
>
> [ side note ]
> I still think we should not be doing any conversion in the SQL_ASCII
> case but this slimmed down patch should be less controversial.
>
--
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] Double sorting split patch
On Tue, Oct 4, 2011 at 12:12 PM, Heikki Linnakangas <
heikki.linnakan...@enterprisedb.com> wrote:
> Can you elaborate the consider-split algorithm? The criteria to select the
> new split over the previously selected one is this:
>
>> ! /*
>> !* If ratio is acceptable, we should compare current split
>> with
>> !* previously selected one. If no split was selected then
>> we select
>> !* current anyway. Between splits of one dimension we
>> search for
>> !* minimal overlap (allowing negative values) and minimal
>> ration
>> !* (between same overlaps. We switch dimension if find
>> less overlap
>> !* (non-negative) or less range with same overlap.
>> !*/
>> ! range = diminfo->upper - diminfo->lower;
>> ! overlap = ((leftUpper) - (rightLower)) / range;
>> ! if (context->first ||
>> ! (context->dim == dimNum &&
>> !(overlap < context->overlap ||
>> ! (overlap == context->overlap && ratio >
>> context->ratio))) ||
>> ! (context->dim != dimNum &&
>> !((range > context->range &&
>> ! non_negative(overlap) <=
>> non_negative(context->overlap)**) ||
>> ! non_negative(overlap) <
>> non_negative(context->overlap)**))
>> ! )
>> ! {
>>
>
> Why are negative overlaps handled differently across dimensions and within
> the same dimension? Your considerSplit algorithm in the SYRCoSE 2011 paper
> doesn't seem to make that distinction.
Yes, I've changed this behaviour after experiments on real-life datasets. On
the datasets where data don't overlap themselfs (points are always such
datasets), non-overlapping splits are frequently possible. In this case
splits tends to be along one dimension, because most distant non-overlapping
splits (i.e. having lowest negative overlap) appears to be in the same
dimension as previous split. Therefore MBRs appear to be very prolonged
along another dimension, that's bad for search. In order to prevent this
behavour I've made transition to another dimension by non-nagative part of
overlap and range. Using range as the split criteria makes MBRs more
quadratic, and using non-negative part of overlap as priority criteria give
to range chance to matter.
--
With best regards,
Alexander Korotkov.
Re: [HACKERS] Re: [COMMITTERS] pgsql: Force strings passed to and from plperl to be in UTF8 encoding.
On Mon, Oct 3, 2011 at 23:35, Amit Khandekar
wrote:
> WHen GetDatabaseEncoding() != PG_UTF8 case, ret will not be equal to
> utf8_str, so pg_verify_mbstr_len() will not get called. That's the
> reason, pg_verify_mbstr_len() is under the ( ret == utf8_str )
> condition.
Consider a latin1 database where utf8_str was a string of ascii
characters. Then no conversion would take place and ret == utf8_str
but the string would be verified by pg_do_encdoing_conversion() and
verified again by your added check :-).
>> It might be worth adding a regression test also...
>
> I could not find any basic pl/perl tests in the regression
> serial_schedule. I am not sure if we want to add just this scenario
> without any basic tests for pl/perl ?
I went ahead and added one in the attached based upon your example.
Look ok to you?
BTW thanks for the patch!
[ side note ]
I still think we should not be doing any conversion in the SQL_ASCII
case but this slimmed down patch should be less controversial.
*** a/src/pl/plperl/GNUmakefile
--- b/src/pl/plperl/GNUmakefile
***
*** 57,63 PSQLDIR = $(bindir)
include $(top_srcdir)/src/Makefile.shlib
! plperl.o: perlchunks.h plperl_opmask.h
plperl_opmask.h: plperl_opmask.pl
@if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi
--- 57,63
include $(top_srcdir)/src/Makefile.shlib
! plperl.o: perlchunks.h plperl_opmask.h plperl_helpers.h
plperl_opmask.h: plperl_opmask.pl
@if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch --with-perl was not specified."; exit 1; fi
*** a/src/pl/plperl/expected/plperl.out
--- b/src/pl/plperl/expected/plperl.out
***
*** 639,641 CONTEXT: PL/Perl anonymous code block
--- 639,651
DO $do$ use warnings FATAL => qw(void) ; my @y; my $x = sort @y; 1; $do$ LANGUAGE plperl;
ERROR: Useless use of sort in scalar context at line 1.
CONTEXT: PL/Perl anonymous code block
+ --
+ -- Make sure strings are validated -- This code may fail in a non-UTF8 database
+ -- if it allows null bytes in strings.
+ --
+ CREATE OR REPLACE FUNCTION perl_zerob() RETURNS TEXT AS $$
+ return "abcd\0efg";
+ $$ LANGUAGE plperlu;
+ SELECT perl_zerob();
+ ERROR: invalid byte sequence for encoding "UTF8": 0x00
+ CONTEXT: PL/Perl function "perl_zerob"
*** a/src/pl/plperl/plperl_helpers.h
--- b/src/pl/plperl/plperl_helpers.h
***
*** 9,14 utf_u2e(const char *utf8_str, size_t len)
--- 9,22
{
char *ret = (char *) pg_do_encoding_conversion((unsigned char *) utf8_str, len, PG_UTF8, GetDatabaseEncoding());
+ /*
+ * when src encoding == dest encoding (PG_UTF8 ==
+ * GetDatabaseEncoding(), pg_do_encoding_conversion() is a noop and
+ * does not verify the string so we need to do it manually
+ */
+ if(GetDatabaseEncoding() == PG_UTF8)
+ pg_verify_mbstr_len(PG_UTF8, utf8_str, len, false);
+
if (ret == utf8_str)
ret = pstrdup(ret);
return ret;
*** a/src/pl/plperl/sql/plperl.sql
--- b/src/pl/plperl/sql/plperl.sql
***
*** 415,417 DO $do$ use strict; my $name = "foo"; my $ref = $$name; $do$ LANGUAGE plperl;
--- 415,426
-- check that we can "use warnings" (in this case to turn a warn into an error)
-- yields "ERROR: Useless use of sort in scalar context."
DO $do$ use warnings FATAL => qw(void) ; my @y; my $x = sort @y; 1; $do$ LANGUAGE plperl;
+
+ --
+ -- Make sure strings are validated -- This code may fail in a non-UTF8 database
+ -- if it allows null bytes in strings.
+ --
+ CREATE OR REPLACE FUNCTION perl_zerob() RETURNS TEXT AS $$
+ return "abcd\0efg";
+ $$ LANGUAGE plperlu;
+ SELECT perl_zerob();
--
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] Double sorting split patch
On 22.09.2011 22:12, Alexander Korotkov wrote:
Patch without that dead code is attached.
Thanks.
Can you elaborate the consider-split algorithm? The criteria to select
the new split over the previously selected one is this:
! /*
!* If ratio is acceptable, we should compare current split with
!* previously selected one. If no split was selected then we
select
!* current anyway. Between splits of one dimension we search for
!* minimal overlap (allowing negative values) and minimal ration
!* (between same overlaps. We switch dimension if find less
overlap
!* (non-negative) or less range with same overlap.
!*/
! range = diminfo->upper - diminfo->lower;
! overlap = ((leftUpper) - (rightLower)) / range;
! if (context->first ||
! (context->dim == dimNum &&
!(overlap < context->overlap ||
! (overlap == context->overlap && ratio >
context->ratio))) ||
! (context->dim != dimNum &&
!((range > context->range &&
! non_negative(overlap) <=
non_negative(context->overlap)) ||
! non_negative(overlap) <
non_negative(context->overlap)))
! )
! {
Why are negative overlaps handled differently across dimensions and
within the same dimension? Your considerSplit algorithm in the SYRCoSE
2011 paper doesn't seem to make that distinction.
--
Heikki Linnakangas
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] Should we get rid of custom_variable_classes altogether?
Tom Lane writes: >> Or do you want to open SET typo.wrogn TO 'foobar' to just work silently? > > Well, right at the moment it *does* work silently, as long as the prefix > is one you listed in custom_variable_classes. I don't think we want to > take that away, and in particular I don't want to assume that every > variable will be declared in advance. It's a fairly safe bet that there > are some apps out there that would be broken by such a requirement. Fair enough I suppose. But I think we could break that requirement if we offer a good enough way out. > At the same time, I'd kind of like to see a facility for declaring such > variables, if only so you could define them to be bool/int/real not just > strings. But this is getting far afield from the immediate proposal, > and no I'm not volunteering to do it. I think we are able to handle that part when dealing with C extension's GUCs, because those are declared in the .so. We only need to add them to the control file, the only road block here used to be c_v_c. What I have in mind for extensions now that c_v_c is out would be to be able to declare any GUC in the control file, with default values, and without forcing extension to handle the GUC in its .so — I don't think we have to change the code beside removing the c_v_c checks here. In the general case, how far exposing DefineCustomBoolVariable and all at the SQL level would get us? Then you could allow the session to add any new GUC by calling that first, SET would bail out if given unknown variable. Yes, it would still break some existing applications, but I think it'd be worth it (and an easy fix too, as I guess most shared variables are going to be used in PL code, and if you ask me this code should now be an extension and the control file would then declare the GUCs). Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- 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] Bug with pg_ctl -w/wait and config-only directories
On mån, 2011-10-03 at 18:44 -0400, Bruce Momjian wrote: > Agreed. You could argue that pg_ctl 9.1 is much better than anything > anyone would be able to craft in a script. And what facts support that argument? Anyway, this comes back to your favorite argument upthread. pg_ctl has had occasional problems in the past. So people have created alternatives that are customized for their particular use case. And some of those init scripts and the like support versions back from 8.1 or 8.2 up to last week. So it will take 2, 3, or 5 years until they'd even consider abandoning their frameworks for, say, a pg_ctl-based solution. And even then there will individual debates on whether it would be worth doing. So, the bottom line is, until now there has been no widespread "forced" use of pg_ctl, outside of Windows and pg_upgrade. PostgreSQL 9.0 was not packaged in any released version of Debian or Ubuntu, so I guess not a lot of people tried pg_upgrade in those configurations. Then again, 9.1 isn't packaged in a released OS version either, of course, so I have no idea why this is coming up exactly now. -- 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] Bug with pg_ctl -w/wait and config-only directories
On mån, 2011-10-03 at 17:12 -0400, Andrew Dunstan wrote: > > On 10/03/2011 04:41 PM, Peter Eisentraut wrote: > > On mån, 2011-10-03 at 15:09 -0400, Bruce Momjian wrote: > >> Why were people not using pg_ctl? Because of the limitations which > >> were fixed in PG 9.1? As Dave already said, windows already has to > >> use pg_ctl. > > Historically, pg_ctl has had a lot of limitations. Just off the top of > > my head, nonstandard ports used to break it, nonstandard socket > > directories used to break it, nonstandard authentication setups used to > > break it, the waiting business was unreliable, the stop modes were weird > > and not flexible enough, the behavior in error cases does not conform to > > LSB init script conventions, there were some race conditions that I > > don't recall the details of right now. And you had to keep a list of > > exactly which of these bugs were addressed in which version. > I'm not sure ancient history helps us much here. Many of these went > away long ago. But some of them are still there. 8.4 is still the packaged version in some popular Linux distributions, and the fabled fixed-it-all version 9.1 was just released a few weeks ago. So in current production environments, pg_ctl is still an occasional liability. > Our job should be to make it better. Yeah, don't get me wrong, let's make it better. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
