Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW
Etsuro Fujita wrote: > While updating the patch, I noticed that in the previous patch, there is > a bug in pushing down parameterized UPDATE/DELETE queries; generic plans > for such queries fail with a can't-happen error. I fixed the bug and > tried to add the regression tests that execute the generic plans, but I > couldn't because I can't figure out how to force generic plans. Is > there any way to do that? I don't know about a way to force it, but if you run the statement six times, it will probably switch to a generic plan. Yours, Laurenz Albe -- 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] Optimization for updating foreign tables in Postgres FDW
On 2015/02/16 12:03, Etsuro Fujita wrote: > I'll update the patch. While updating the patch, I noticed that in the previous patch, there is a bug in pushing down parameterized UPDATE/DELETE queries; generic plans for such queries fail with a can't-happen error. I fixed the bug and tried to add the regression tests that execute the generic plans, but I couldn't because I can't figure out how to force generic plans. Is there any way to do that? Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Install shared libs in lib/ and bin/ with MSVC (Was: install libpq.dll in bin directory on Windows / Cygwin)
On Tue, Mar 3, 2015 at 8:36 PM, Asif Naeem wrote:
> Thank you Michael. I have looked the patch.
Thanks for the review!
> Overall logic looks good to me,
> I have checked it with MSVC{2013,2008}. It works for MSVC 2013 but fail for
> MSVC 2008, I think the condition "if ($proj =~
> qr{ResourceCompile\s*Include="([^"]+)"})" is not going to work for MSVC2008
> and MSVC2005 i.e.
> [...]
Thanks for the details, my patch is definitely missing vcproj entries.
Note that I don't have yet an environment with MSVC 2008 or similar, I
just got 2010 on my box for now. So you will have to wait until I have
a fresh environment to get an updated patch...
> It seems that search criteria can be narrowed to skip reading related
> Makefile for SO_MAJOR_VERSION string when VS project file is related to
> StaticLibrary or Application.
Well, agreed, and the patch takes care of that for vcxproj files by
only installing shared libraries if they use DynamicLibrary. Perhaps
you have in mind a better logic?
> Although this patch is going to make MSVC
> build consistent with Cygwin and MinGW build, following files seems
> redundant now, is there any use for them other than backward compatibility ?
> i.e.
>> inst\lib\libpq.dll
>> inst\lib\libpgtypes.dll
>> inst\lib\libecpg_compat.dll
>> inst\lib\libecpg.dll
Indeed, I haven't noticed them. But we can simply remove them as they
are installed in bin/ as well with this patch, it seems better for
consistency with MinGW and Cygwin in any case.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Configurable location for extension .control files
18.02.2015, 01:49, Jim Nasby kirjoitti: > On 2/17/15 4:39 PM, Oskari Saarenmaa wrote: >> 10.06.2013, 17:51, Dimitri Fontaine kirjoitti: >>> Andres Freund writes: > In any case, no packager is going to ship an insecure-by-default > configuration, which is what Dimitri seems to be fantasizing would > happen. It would have to be local option to relax the permissions > on the directory, no matter where it is. *I* don't want that at all. All I'd like to have is a postgresql.conf option specifying additional locations. >>> >>> Same from me. I think I would even take non-plural location. >> >> Here's a patch to allow overriding extension installation directory. >> The patch allows superusers to change it at runtime, but we could also >> restrict it to postgresql.conf if needed. I don't really see a point in >> restricting it (or not implementing the option at all) as the superuser >> can already load custom C code and sql from anywhere in the filesystem; >> not having configurable extension directory just makes it harder to test >> extensions and to create private clusters of PG data in a custom >> location without using custom binaries. > > I'm interested in this because it could potentially make it possible to > install SQL extensions without OS access. (My understanding is there's > some issue with writing the extension files even if LIBDIR is writable > by the OS account). I'm not sure this patch makes extensions without OS access any easier to implement; you still need to write the files somewhere, and someone needs to set up the cluster with a nonstandard extension path. >> I don't think anyone should be packaging and shipping PG with >> extension_directory set, but we really should allow it for superusers >> and postgresql.conf so people can test extensions without hacks like >> this: https://github.com/ohmu/pgmemcache/blob/master/localtests.sh#L23 > > FWIW I'd like to see is breaking the cluster setup/teardown capability > in pg_regress into it's own tool. It wouldn't solve the extension test > problem, but it would eliminate the need for most of what that script is > doing, and it would do it more robustly. It would make it very easy to > unit test things with frameworks other than pg_regress. This would certainly be useful. I can try to do something about it if we get configurable extension path supported. The patch is now in https://commitfest.postgresql.org/5/170/ / Oskari -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add LINE: hint when schemaname.typename is a non-existent schema
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, failed Spec compliant: not tested Documentation:not tested Tom suggested few changes already which I too think author needs to address. So marking it "Waiting on Author". However, I see following, example does not work well. postgres=# create or replace function f1(a abc.test.id%type) returns int as $$ select 1; $$ language sql; ERROR: schema "abc" does not exist Is that expected? I guess we need it at all places in parse_*.c where we will look for namespace. Please fix. Also, like Tom's suggestion on make_oper_cache_key, can we push down this inside func_get_detail() as well, just to limit it for namespace lookup? However, patch is not getting applied cleanly on latest sources. Need rebase. > On Tom comments on parse_utilcmd.c: I guess the block is moved after the pstate and CreateStmtContext are setup properly. I guess, we can move just after pstate setup, so that it will result into minimal changes? Can we have small test-case? Or will it be too much for this feature? The new status of this patch is: Waiting on Author -- 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] How about to have relnamespace and relrole?
On 3/3/15 8:04 PM, Kyotaro HORIGUCHI wrote: >Note: The OID alias types don't sctrictly comply the transaction > isolation rules so do not use them where exact transaction > isolation on the values of these types has a > significance. Likewise, since they look as simple constants to > planner so you might get slower plans than the queries joining > the system tables correnspond to the OID types. Might I suggest: Note: The OID alias types do not completely follow transaction isolation rules. The planner also treats them as simple constants, which may result in sub-optimal planning. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi wrote: > + foreach(line, parsed_hba_lines) > > In the above for loop it is better to add "check_for_interrupts" to > avoid it looping > if the parsed_hba_lines are more. Updated patch is attached with the addition of check_for_interrupts in the for loop. Regards, Hari Babu Fujitsu Australia Catalog_view_to_HBA_settings_patch_V7.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes
On Wed, Mar 4, 2015 at 12:41 AM, Syed, Rahila wrote:
> Please find attached updated patch with WAL replay error fixed. The patch
> follows chunk ID approach of xlog format.
(Review done independently of the chunk_id stuff being good or not,
already gave my opinion on the matter).
* readRecordBufSize is set to the new buffer size.
- *
+
The patch has some noise diffs.
You may want to change the values of BKPBLOCK_WILL_INIT and
BKPBLOCK_SAME_REL to respectively 0x01 and 0x02.
+ uint8 chunk_id = 0;
+ chunk_id |= XLR_CHUNK_BLOCK_REFERENCE;
Why not simply that:
chunk_id = XLR_CHUNK_BLOCK_REFERENCE;
+#define XLR_CHUNK_ID_DATA_SHORT255
+#define XLR_CHUNK_ID_DATA_LONG 254
Why aren't those just using one bit as well? This seems inconsistent
with the rest.
+ if ((blk->with_hole == 0 && blk->hole_offset != 0) ||
+ (blk->with_hole == 1 && blk->hole_offset <= 0))
In xlogreader.c blk->with_hole is defined as a boolean but compared
with an integer, could you remove the ==0 and ==1 portions for
clarity?
- goto err;
+ goto err;
}
}
-
if (remaining != datatotal)
This gathers incorrect code alignment and unnecessary diffs.
typedef struct XLogRecordBlockHeader
{
+ /* Chunk ID precedes */
+
uint8 id;
What prevents the declaration of chunk_id as an int8 here instead of
this comment? This is confusing.
> Following are brief measurement numbers.
> WAL
> FPW compression on 122.032 MB
> FPW compression off 155.239 MB
> HEAD 155.236 MB
What is the test run in this case? How many block images have been
generated in WAL for each case? You could gather some of those numbers
with pg_xlogdump --stat for example.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] a fast bloat measurement tool (was Re: Measuring relation free space)
On Mon, Feb 23, 2015 at 7:11 AM, Tomas Vondra wrote: > On 28.1.2015 05:03, Abhijit Menon-Sen wrote: > > At 2015-01-27 17:00:27 -0600, jim.na...@bluetreble.com wrote: > >> > Otherwise, the code looks OK to me. Now, there are a few features I'd > like to have for production use (to minimize the impact): > > 1) no index support :-( > >I'd like to see support for more relation types (at least btree >indexes). Are there any plans for that? Do we have an idea on how to >compute that? > > 2) sampling just a portion of the table > >For example, being able to sample just 5% of blocks, making it less >obtrusive, especially on huge tables. Interestingly, there's a >TABLESAMPLE patch in this CF, so maybe it's possible to reuse some >of the methods (e.g. functions behind SYSTEM sampling)? > > 3) throttling > >Another feature minimizing impact of running this on production might >be some sort of throttling, e.g. saying 'limit the scan to 4 MB/s' >or something along those lines. > I think these features could be done separately if anybody is interested. The patch in its proposed form seems useful to me. > 4) prefetch > >fbstat_heap is using visibility map to skip fully-visible pages, >which is nice, but if we skip too many pages it breaks readahead >similarly to bitmap heap scan. I believe this is another place where >effective_io_concurrency (i.e. prefetch) would be appropriate. > Good point. We can even think of using the technique used by Vacuum which is skip only when we can skip atleast SKIP_PAGES_THRESHOLD pages. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] a fast bloat measurement tool (was Re: Measuring relation free space)
On Fri, Dec 26, 2014 at 1:08 PM, Abhijit Menon-Sen wrote: > > At 2014-09-25 15:40:11 +0530, a...@2ndquadrant.com wrote: > > > > All right, then I'll post a version that addresses Amit's other > > points, adds a new file/function to pgstattuple, acquires content > > locks, and uses HeapTupleSatisfiesVacuum, hint-bit setting and all. > > Sorry, I forgot to post this patch. It does what I listed above, and > seems to work fine (it's still quite a lot faster than pgstattuple > in many cases). > I think one of the comment is not handled in latest patch. 5. Don't we need the handling for empty page (PageIsEmpty()) case? I think we should have siimilar for PageIsEmpty() as you have done for PageIsNew() in your patch. + stat.tuple_count = vac_estimate_reltuples(rel, false, nblocks, scanned, + stat.tuple_count); Here for scanned tuples, you have used the live tuple counter whereas I think we need to count HEAPTUPLE_INSERT_IN_PROGRESS and HEAPTUPLE_DELETE_IN_PROGRESS and HEAPTUPLE_RECENTLY_DEAD. Refer the similar logic in Vacuum. Although I am not in favor of using HeapTupleSatisfiesVacuum(), however if you want to use it then lets be consistent with what Vacuum does for estimation of tuples. > A couple of remaining questions: > > 1. I didn't change the handling of LP_DEAD items, because the way it is >now matches what pgstattuple counts. I'm open to changing it, though. >Maybe it makes sense to count LP_DEAD items iff lp_len != 0? Or just >leave it as it is? I think it doesn't matter much. > > 2. I changed the code to acquire the content locks on the buffer, as >discussed, but it still uses HeapTupleSatisfiesVacuum. Amit suggested >using HeapTupleSatisfiesVisibility, but it's not clear to me why that >would be better. I welcome advice in this matter. > The reason to use HeapTupleSatisfiesVacuum() is that it helps us in freezing and some other similar stuff which is required by Vacuum. Also it could be beneficial if we want take specific action for various result codes of Vaccum. I think as this routine mostly gives the estimated count, so it might not matter much even if we use HeapTupleSatisfiesVacuum(), however it is better to be consistent with pgstat_heap() in this case. Do you have any reason for using HeapTupleSatisfiesVacuum() rather than what is used in pgstat_heap()? >(If anything, I should use HeapTupleIsSurelyDead, which doesn't set >any hint bits, but which I earlier rejected as too conservative in >its dead/not-dead decisions for this purpose.) > > (I've actually patched the pgstattuple.sgml docs as well, but I want to > re-read that to make sure it's up to date, and didn't want to wait to > post the code changes.) > Sure, post the same when it is ready. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Bug in pg_dump
On Wed, Mar 4, 2015 at 6:48 AM, Peter Eisentraut wrote: > - set up basic scaffolding for TAP tests in src/bin/pg_dump Agreed. > - write a Perl function that can create an extension on the fly, given > name, C code, SQL code I am perplex about that. Where would the SQL code or C code be stored? In the pl script itself? I cannot really see the advantage to generate automatically the skeletton of an extension based on some C or SQL code in comparison to store the extension statically as-is. Adding those extensions in src/test/modules is out of scope to not bloat it, so we could for example add such test extensions in t/extensions/ or similar, and have prove_check scan this folder, then install those extensions in the temporary installation. > - add to the proposed t/001_dump_test.pl code to write the extension > - add that test to the pg_dump test suite > Eventually, the dump-and-restore routine could also be refactored, but > as long as we only have one test case, that can wait. Agreed on all those points. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] chkpass with RANDOMIZE_ALLOCATED_MEMORY
Thank you Tom, Thank you Amit. Regards, Muhammad Asif Naeem On Wed, Mar 4, 2015 at 9:30 AM, Tom Lane wrote: > Amit Kapila writes: > > On Sat, Feb 14, 2015 at 10:26 PM, Tom Lane wrote: > >> It's not a false alarm, unfortunately, because chkpass_in actually does > >> give different results from one call to the next. We could fix the > aspect > >> of that involving failing to zero out unused bytes (which it appears was > >> introduced by sloppy replacement of strncpy with strlcpy). But we can't > >> really do anything about the dependency on random(), because that's part > >> of the fundamental specification of the data type. It was a bad idea, > >> no doubt, to design the input function to do this; but we're stuck with > >> it now. > > > It seems to me that fix for this issue has already been committed > > (commit-id: 80986e85). So isn't it better to mark as Committed in > > CF app [1] or are you expecting anything more related to this issue? > > > [1]: https://commitfest.postgresql.org/4/144/ > > Ah, I didn't realize there was a CF entry for it, I think. Yeah, > I think we committed as much as we should of this, so I marked the > CF entry as committed. > > regards, tom lane >
Re: [HACKERS] Comparing primary/HS standby in tests
On Wed, Mar 4, 2015 at 12:49 AM, Andres Freund wrote:
> I every now and then run installcheck against a primary, verify that
> replay works without errors, and then compare pg_dumpall from both
> clusters. Unfortunately that currently requires hand inspection of
> dumps, there are differences like:
> -SELECT pg_catalog.setval('default_seq', 1, true);
> +SELECT pg_catalog.setval('default_seq', 33, true);
> Does anybody have a good idea how to get rid of that difference? One way
> to do that would be to log the value the standby is sure to have - but
> that's not entirely trivial.
SEQ_LOG_VALS has been added some time ago, so perhaps time have
changed and we could live without it:
commit: 741510521caea7e1ca12b4db0701bbc2db346a5f
author: Vadim B. Mikheev
date: Thu, 30 Nov 2000 01:47:33 +
XLOG stuff for sequences.
CommitDelay in guc.c
However performance is really a problem, for example with the patch
attached and the following test case:
DO $$DECLARE count integer; count2 integer;
begin
for count in 1 .. 100
loop
select nextval('toto') into count2;
end loop;
END$$;
Patched, this takes 9.5ms and generates 191 MB of WAL on my laptop.
With master unpatched, this generates 6MB of WAL (records are divided
by 32) and takes 7.5s.
There are a couple of other possibilities we could consider as well:
1) Trick pg_dump such as it does not dump the current value of master
but one consistent with what a standby would expect. We would need
then something like nextval_standby() or similar.
2) Filter out lines with pg_catalog.setval in a home-made wrapper.
> I'd very much like to add a automated test like this to the tree, but I
> don't see a way to do that sanely without a comparison tool...
That's definitely worth having IMO.
Regards,
--
Michael
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 0070c4f..da503fe 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -42,13 +42,6 @@
/*
- * We don't want to log each fetching of a value from a sequence,
- * so we pre-log a few fetches in advance. In the event of
- * crash we can lose (skip over) as many values as we pre-logged.
- */
-#define SEQ_LOG_VALS 32
-
-/*
* The "special area" of a sequence's buffer page looks like this.
*/
#define SEQ_MAGIC 0x1717
@@ -206,11 +199,6 @@ DefineSequence(CreateSeqStmt *seq)
coldef->colname = "cache_value";
value[i - 1] = Int64GetDatumFast(new.cache_value);
break;
- case SEQ_COL_LOG:
-coldef->typeName = makeTypeNameFromOid(INT8OID, -1);
-coldef->colname = "log_cnt";
-value[i - 1] = Int64GetDatum((int64) 0);
-break;
case SEQ_COL_CYCLE:
coldef->typeName = makeTypeNameFromOid(BOOLOID, -1);
coldef->colname = "is_cycled";
@@ -297,7 +285,6 @@ ResetSequence(Oid seq_relid)
seq = (Form_pg_sequence) GETSTRUCT(tuple);
seq->last_value = seq->start_value;
seq->is_called = false;
- seq->log_cnt = 0;
/*
* Create a new storage file for the sequence. We want to keep the
@@ -538,13 +525,11 @@ nextval_internal(Oid relid)
maxv,
minv,
cache,
-log,
fetch,
last;
int64 result,
next,
rescnt = 0;
- bool logit = false;
/* open and AccessShareLock sequence */
init_sequence(relid, &elm, &seqrel);
@@ -579,7 +564,6 @@ nextval_internal(Oid relid)
maxv = seq->max_value;
minv = seq->min_value;
fetch = cache = seq->cache_value;
- log = seq->log_cnt;
if (!seq->is_called)
{
@@ -587,35 +571,7 @@ nextval_internal(Oid relid)
fetch--;
}
- /*
- * Decide whether we should emit a WAL log record. If so, force up the
- * fetch count to grab SEQ_LOG_VALS more values than we actually need to
- * cache. (These will then be usable without logging.)
- *
- * If this is the first nextval after a checkpoint, we must force a new
- * WAL record to be written anyway, else replay starting from the
- * checkpoint would fail to advance the sequence past the logged values.
- * In this case we may as well fetch extra values.
- */
- if (log < fetch || !seq->is_called)
- {
- /* forced log to satisfy local demand for values */
- fetch = log = fetch + SEQ_LOG_VALS;
- logit = true;
- }
- else
- {
- XLogRecPtr redoptr = GetRedoRecPtr();
-
- if (PageGetLSN(page) <= redoptr)
- {
- /* last update of seq was before checkpoint */
- fetch = log = fetch + SEQ_LOG_VALS;
- logit = true;
- }
- }
-
- while (fetch)/* try to fetch cache [+ log ] numbers */
+ while (fetch)/* try to fetch cache numbers */
{
/*
* Check MAXVALUE for ascending sequences and MINVALUE for descending
@@ -670,7 +626,6 @@ nextval_internal(Oid relid)
fetch--;
if (rescnt < cache)
{
- log--;
rescnt++;
last = next;
if (rescnt == 1) /* if it's first result - */
@@ -678,9 +633,6 @@ nextval_internal(Oid relid)
}
}
- log -= fetch;/* adjust for any unfetched numbers */
- Assert(log >= 0);
-
/* save info in local cache */
elm->last = result; /* last returned
Re: [HACKERS] chkpass with RANDOMIZE_ALLOCATED_MEMORY
Amit Kapila writes: > On Sat, Feb 14, 2015 at 10:26 PM, Tom Lane wrote: >> It's not a false alarm, unfortunately, because chkpass_in actually does >> give different results from one call to the next. We could fix the aspect >> of that involving failing to zero out unused bytes (which it appears was >> introduced by sloppy replacement of strncpy with strlcpy). But we can't >> really do anything about the dependency on random(), because that's part >> of the fundamental specification of the data type. It was a bad idea, >> no doubt, to design the input function to do this; but we're stuck with >> it now. > It seems to me that fix for this issue has already been committed > (commit-id: 80986e85). So isn't it better to mark as Committed in > CF app [1] or are you expecting anything more related to this issue? > [1]: https://commitfest.postgresql.org/4/144/ Ah, I didn't realize there was a CF entry for it, I think. Yeah, I think we committed as much as we should of this, so I marked the CF entry as committed. 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] chkpass with RANDOMIZE_ALLOCATED_MEMORY
On Sat, Feb 14, 2015 at 10:26 PM, Tom Lane wrote: > > Asif Naeem writes: > > It is been observed on RANDOMIZE_ALLOCATED_MEMORY enabled PG95 build that > > chkpass is failing because of uninitialized memory and seems showing false > > alarm. > > It's not a false alarm, unfortunately, because chkpass_in actually does > give different results from one call to the next. We could fix the aspect > of that involving failing to zero out unused bytes (which it appears was > introduced by sloppy replacement of strncpy with strlcpy). But we can't > really do anything about the dependency on random(), because that's part > of the fundamental specification of the data type. It was a bad idea, > no doubt, to design the input function to do this; but we're stuck with > it now. > It seems to me that fix for this issue has already been committed (commit-id: 80986e85). So isn't it better to mark as Committed in CF app [1] or are you expecting anything more related to this issue? [1]: https://commitfest.postgresql.org/4/144/ With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Proposal: knowing detail of config files via SQL
* Stephen Frost (sfr...@snowman.net) wrote: > pg_start_backup, pg_stop_backup, pg_switch_xlog, pg_rotate_logfile, > pg_create_restore_point, pg_xlog_replay_pause, lo_import, lo_export, > and pg_xlog_replay_resume. Meh, that list was too hastily copied and pasted from my earlier email. lo_import and lo_export were *not* included in the role attributes list, nor would I advocate making them GRANT'able. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Obsolete SnapshotNow reference within snapbuild.c
On Wed, Mar 4, 2015 at 10:31 AM, Peter Geoghegan wrote: > SnapBuildCommitTxn() has what I gather is an obsolete reference to > SnapshotNow(). Attached patch corrects this. Pushed. Thanks! Regards, -- Fujii Masao -- 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] Re: Compression of full-page-writes
On Tue, Mar 3, 2015 at 9:34 AM, Michael Paquier wrote: > On Tue, Mar 3, 2015 at 9:24 AM, Andres Freund wrote: >> On 2015-03-03 08:59:30 +0900, Michael Paquier wrote: >>> Already mentioned upthread, but I agree with Fujii-san here: adding >>> information related to the state of a block image in >>> XLogRecordBlockHeader makes little sense because we are not sure to >>> have a block image, perhaps there is only data associated to it, and >>> that we should control that exclusively in XLogRecordBlockImageHeader >>> and let the block ID alone for now. >> >> This argument doesn't make much sense to me. The flag byte could very >> well indicate 'block reference without image following' vs 'block >> reference with data + hole following' vs 'block reference with >> compressed data following'. > > Information about the state of a block is decoupled with its > existence, aka in the block header, we should control if: > - record has data > - record has a block > And in the block image header, we control if the block is: > - compressed or not > - has a hole or not. Are there any other flag bits that we should or are planning to add into WAL header newly, except the above two? If yes and they are required by even a block which doesn't have an image, I will change my mind and agree to add something like chunk ID to a block header. But I guess the answer of the question is No. Since the flag bits now we are thinking to add are required only by a block having an image, adding them into a block header (instead of block image header) seems a waste of bytes in WAL. So I concur with Michael. Regards, -- Fujii Masao -- 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] Reduce pinning in btree indexes
Kevin Grittner wrote: > [need to check performance more] It looks like the remaining performance regression was indeed a result of code alignment. I found two "paranoia" assignments I had accidentally failed to put back with the rest of the mark/restore optimization; after that trivial change the patched version is ever-so-slightly faster than master on all tests. Average of 3 `make check-world` runs: master: 336.288 seconds patch: 332.237 seconds Average of 6 `make check` runs: master: 25.409 seconds patch: 25.270 seconds The following were all run 12 times, the worst 2 dropped, the rest averaged: Kyotaro-san's 1m mark "worst case" benchmark: master: 962.581 ms, 6.765 stdev patch: 947.518 ms, 3.584 stdev Kyotaro-san's 500k mark, 500k restore "worst case" benchmark: master: 1366.639 ms, 5.314 stdev patch: 1363.844 ms, 5.896 stdev pgbench -i -s 16 pgbench / pgbench -c 16 -j 4 -T 500 pgbench master: 265.011 tps, 4.952 stdev patch: 267.164 tps, 9.094 stdev How do people feel about the idea of me pushing this for 9.5 (after I clean up all the affected comments and README files)? I know this first appeared in the last CF, but the footprint is fairly small and the only user-visible behavior change is that a btree index scan of a WAL-logged table using an MVCC snapshot no longer blocks a vacuum indefinitely. (If there are objections I will move this to the first CF for the next release.) src/backend/access/nbtree/nbtree.c| 50 +--- src/backend/access/nbtree/nbtsearch.c | 141 +- src/backend/access/nbtree/nbtutils.c | 58 ++ src/include/access/nbtree.h | 36 - 4 files changed, 201 insertions(+), 84 deletions(-) -- Kevin Grittner EDB: 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] POLA violation with \c service=
On Mon, Mar 2, 2015 at 5:05 PM, David Fetter wrote: > So just to clarify, are you against back-patching the behavior change, > or the addition to src/common? Mostly the latter. -- 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] Bootstrap DATA is a pita
On Sat, Feb 21, 2015 at 11:34 AM, Tom Lane wrote:
> Andres Freund writes:
>> On 2015-02-20 22:19:54 -0500, Peter Eisentraut wrote:
>>> On 2/20/15 8:46 PM, Josh Berkus wrote:
Or what about just doing CSV?
>
>>> I don't think that would actually address the problems. It would just
>>> be the same format as now with different delimiters.
>
>> Yea, we need hierarchies and named keys.
>
> Yeah. One thought though is that I don't think we need the "data" layer
> in your proposal; that is, I'd flatten the representation to something
> more like
>
> {
> oid => 2249,
> oiddefine => 'CSTRINGOID',
> typname => 'cstring',
> typlen => -2,
> typbyval => 1,
> ...
> }
Even this promises to vastly increase the number of lines in the file,
and make it harder to compare entries by grepping out some common
substring. I agree that the current format is a pain in the tail, but
pg_proc.h is >5k lines already. I don't want it to be 100k lines
instead.
--
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] INSERT ... ON CONFLICT UPDATE and logical decoding
On Tue, Mar 3, 2015 at 3:13 AM, Andres Freund wrote: >> toast_save_datum() is called with a heap_insert() call before heap >> insertion for the tuple proper. We're relying on the assumption that >> if there is no immediate super deletion record, things are fine. We >> cannot speculatively insert into catalogs, BTW. > > I fail to see what prohibits e.g. another catalog modifying transaction > to commit inbetween and distribute a new snapshot. SnapBuildDistributeNewCatalogSnapshot()/REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT does look like a problematic case. It's problematic specifically because it involves some xact queuing a change to *some other* xact - we cannot assume that this won't happen between WAL-logging within heap_insert() and heap_delete(). Can you think of any other such cases? I think that REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID should be fine. REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID is not an issue either. In both cases I believe my assumption does not break because there won't be writes to system catalogs between the relevant heap_insert() and heap_delete() calls, which are tightly coupled, and also because speculative insertion into catalogs is unsupported. That just leaves the non-"*CHANGE_INTERAL_* "(regular DML) cases, which should also be fine. As for SnapBuildDistributeNewCatalogSnapshot(), I'm open to suggestions. Do you see any opportunity around making assumptions about heavyweight locking making the interleaving of some REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT change between a REORDER_BUFFER_CHANGE_INTERNAL_INSERT change and a REORDER_BUFFER_CHANGE_INTERNAL_DELETE change? AFAICT, that's all this approach really needs to worry about (or the interleaving of something else not under the control of speculative insertion - doesn't have to be a REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT, that's just the most obvious problematic case). >> Why should we see a REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID case >> next, in the case that we need to care about (when the tuple was super >> deleted immediately afterwards)? > > It's irrelevant whether you care about it or not. Your > ReorderBufferIterTXNNext() consumes a change that needs to actually be > processed. It could very well be something important like a new > snapshot. But it is actually processed, in the next iteration (we avoid calling ReorderBufferIterTXNNext() at the top of the loop if it has already been called for that iteration as part of peeking ahead). AFAICT all that is at issue is the safety of one particular assumption I've made: that it is safe to assume that there will never be a super deletion in the event of not seeing a super deletion change immediately following a speculative insertion change within some xact when decoding. It's still going to be processed if it's something else. The implementation will, however, fail to consolidate the speculative insertion change and super deletion change if my assumption is ever faulty. This whole approach doesn't seem to be all that different to how there is currently coordination within ReorderBufferCommit() between TOAST-related changes, and changes to the relation proper. In fact, I've now duplicated the way the IsToastRelation() case performs "dlist_delete(&change->node)" in order to avoid chunk reuse in the event of spilling to disk. Stress testing by decreasing "max_changes_in_memory" to 1 does not exhibit broken behavior, I believe (although that does break the test_decoding regression tests on the master branch, FWIW). Also, obviously I have not considered the SnapBuildDistributeNewCatalogSnapshot() case too closely. I attach an updated patch that I believe at least handles the serialization aspects correctly, FYI. Note that I assert that REORDER_BUFFER_CHANGE_INTERNAL_DELETE follows a REORDER_BUFFER_CHANGE_INTERNAL_INSERT, so if you can find a way to break the assumption it should cause an assertion failure, which is something to look out for during testing. >> While what I have here *is* very ugly, I see no better alternative. By >> doing what you suggest, we'd be finding a special case way of safely >> violating the general "WAL log-before-data" rule. > > No, it won't. We don't WAL log modifications that don't need to persist > in a bunch of places. It'd be perfectly fine to start upsert with a > 'acquiring a insertion lock' record that pretty much only contains the > item pointer (and a FPI if necessary to prevent torn pages). And then > only log the full new record once it's sure that the whole thing needs > to survive. Redo than can simply clean out the size touched by the > insertion lock. That seems like a lot of work in the general case to not do something that will only very rarely need to occur anyway. The optimistic approach of value locking scheme #2 has a small race that is detected (which necessitates super deletion), but that will only very rarely be required. Also, there is value in making super deletion (and speculative insertion) as close as possible
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
> Obviously FDW can add multiple paths at a time, like GetForeignPaths, > so IMO it should be renamed to GetForeignJoinPaths, with plural form. > > In addition to that, new member of RelOptInfo, fdw_handler, should be > initialized explicitly in build_simple_rel. > > Please see attached a patch for these changes. > Thanks for your checks. Yep, the name of FDW handler should be ...Paths(), instead of Path(). The attached one integrates Hanada-san's updates. -- NEC OSS Promotion Center / PG-Strom Project KaiGai Kohei > -Original Message- > From: Shigeru Hanada [mailto:shigeru.han...@gmail.com] > Sent: Tuesday, March 03, 2015 9:26 PM > To: Kaigai Kouhei(海外 浩平) > Cc: Robert Haas; Tom Lane; pgsql-hackers@postgreSQL.org > Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom > Plan API) > > Kaigai-san, > > The v6 patch was cleanly applied on master branch. I'll rebase my > patch onto it, but before that I have a comment about name of the new > FDW API handler GetForeignJoinPath. > > Obviously FDW can add multiple paths at a time, like GetForeignPaths, > so IMO it should be renamed to GetForeignJoinPaths, with plural form. > > In addition to that, new member of RelOptInfo, fdw_handler, should be > initialized explicitly in build_simple_rel. > > Please see attached a patch for these changes. > > I'll review the v6 path afterwards. > > > 2015-03-03 20:20 GMT+09:00 Kouhei Kaigai : > > Sorry, I misoperated on patch creation. > > Attached one is the correct version. > > -- > > NEC OSS Promotion Center / PG-Strom Project > > KaiGai Kohei > > > > > >> -Original Message- > >> From: pgsql-hackers-ow...@postgresql.org > >> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Kouhei Kaigai > >> Sent: Tuesday, March 03, 2015 6:31 PM > >> To: Kaigai Kouhei(海外 浩平); Robert Haas > >> Cc: Tom Lane; pgsql-hackers@postgreSQL.org; Shigeru Hanada > >> Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan > >> API) > >> > >> The attached version of custom/foreign-join interface patch > >> fixes up the problem reported on the join-pushdown support > >> thread. > >> > >> The previous version referenced *_ps_tlist on setrefs.c, to > >> check whether the Custom/ForeignScan node is associated with > >> a particular base relation, or not. > >> This logic considered above nodes performs base relation scan, > >> if *_ps_tlist is valid. However, it was incorrect in case when > >> underlying pseudo-scan relation has empty targetlist. > >> Instead of the previous logic, it shall be revised to check > >> scanrelid itself. If zero, it means Custom/ForeignScan node is > >> not associated with a particular base relation, thus, its slot > >> descriptor for scan shall be constructed based on *_ps_tlist. > >> > >> > >> Also, I noticed a potential problem if CSP/FDW driver want to > >> displays expression nodes using deparse_expression() but > >> varnode within this expression does not appear in the *_ps_tlist. > >> For example, a remote query below shall return rows with two > >> columns. > >> > >> SELECT atext, btext FROM tbl_a, tbl_b WHERE aid = bid; > >> > >> Thus, ForeignScan will perform like as a scan on relation with > >> two columns, and FDW driver will set two TargetEntry on the > >> fdw_ps_tlist. If FDW is designed to keep the join condition > >> (aid = bid) using expression node form, it is expected to be > >> saved on custom/fdw_expr variable, then setrefs.c rewrites the > >> varnode according to *_ps_tlist. > >> It means, we also have to add *_ps_tlist both of "aid" and "bid" > >> to avoid failure on variable lookup. However, these additional > >> entries changes the definition of the slot descriptor. > >> So, I adjusted ExecInitForeignScan and ExecInitCustomScan to > >> use ExecCleanTypeFromTL(), not ExecTypeFromTL(), when it construct > >> the slot descriptor based on the *_ps_tlist. > >> It expects CSP/FDW drivers to add target-entries with resjunk=true, > >> if it wants to have additional entries for variable lookups on > >> EXPLAIN command. > >> > >> Fortunately or unfortunately, postgres_fdw keeps its remote query > >> in cstring form, so it does not need to add junk entries on the > >> fdw_ps_tlist. > >> > >> Thanks, > >> -- > >> NEC OSS Promotion Center / PG-Strom Project > >> KaiGai Kohei > >> > >> > >> > -Original Message- > >> > From: pgsql-hackers-ow...@postgresql.org > >> > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Kouhei Kaigai > >> > Sent: Sunday, February 15, 2015 11:01 PM > >> > To: Kaigai Kouhei(海外 浩平); Robert Haas > >> > Cc: Tom Lane; pgsql-hackers@postgreSQL.org; Shigeru Hanada > >> > Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan > >> > API) > >> > > >> > The attached patch is a rebased version of join replacement with > >> > foreign-/custom-scan. Here is no feature updates at this moment > >> > but SGML documentation is added (according to Michael's comment). > >> > > >> > This infrastructure allows
Re: [HACKERS] Idea: closing the loop for "pg_ctl reload"
On March 3, 2015 06:34:33 PM Jim Nasby wrote: > On 3/3/15 5:24 PM, Jan de Visser wrote:> On March 3, 2015 04:57:58 PM > Jim Nasby wrote: > >> On 3/3/15 11:48 AM, Andres Freund wrote: > >>> I'm saying that you'll need a way to notice that a reload was > processed > >> > or not. And that can't really be the message itself, there has to be > >> > some other field; like the timestamp Tom proposes. > >> > >> Ahh, right. We should make sure we don't go brain-dead if the system > >> clock moves backwards. I assume we couldn't just fstat the file... > > > > The timestamp field is already there (in my patch). It gets populated > when the > > server starts and repopulated by SIGHUP_handler. It's the only timestamp > > pg_ctl pays attention to. > > What happens if someone does a reload sometime in the future (perhaps > because they've messed with the system clock for test purposes). Do we > still detect a new reload? Good point, and I had that scenario in the back of my head. What I do right now is read the 'last reload' timestamp from postmaster.pid (which can be the same as the startup time, since I make postmaster write an initial timestamp when it starts), send the SIGHUP, and wait until I read a later one or until timeout. What I could do is wait until I read a *different* one, and not just a later one. In that case you're only SOL if you manage to time it just so that your new server time is *exactly* the same as your old server time when you did your last reload (or startup). But even in that case it would time out and recommend you check the log. That whole error message thing has one gotcha BTW; it's not hard, it's just that I have to find a way to make it bubble up from guc_file.l. Triggering an error was just changing the return value from void to bool, but returning the error string would mean returning a char buffer which would then need to be freed by other callers of ProcessConfig etc etc. * /me scratches head But I could just rename the current ProcessConfig, make it return a buffer, and have a *new*, void, ProcessConfig which calls the renamed function and just discards the result. As an aside: I always found it interesting how feature threads "explode" around here. But it figures if even something as "simple" as this gets such detailed input. Definitely something to get used to... -- 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] MD5 authentication needs help
Bruce, all, * Bruce Momjian (br...@momjian.us) wrote: > It feels like MD5 has accumulated enough problems that we need to start > looking for another way to store and pass passwords. The MD5 problems > are: > > 1) MD5 makes users feel uneasy (though our usage is mostly safe) > > 2) The per-session salt sent to the client is only 32-bits, meaning > that it is possible to reply an observed MD5 hash in ~16k connection > attempts. > > 3) Using the user name for the MD5 storage salt allows the MD5 stored > hash to be used on a different cluster if the user used the same > password. > > 4) Using the user name for the MD5 storage salt causes the renaming of > a user to break the stored password. > > For these reasons, it is probably time to start thinking about a > replacement that fixes these issues. We would keep MD5 but recommend > a better option. For more background, I'd suggest taking a look at this recent thread: CA+TgmoaWdkNBT4mNZ+wf=fgjd7av9bq7ntsvcha7yeox0ly...@mail.gmail.com Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] How about to have relnamespace and relrole?
Hello, I attached the latest patches missing in the previous mail.
Thanks for pointing Jeevan.
0001-Add-regrole_v4.patch
0002-Add-regnamespace_v4.patch
Jim> BTW, I think the potential for MVCC issues should be mentioned in the
Jim> docs (http://www.postgresql.org/docs/devel/static/datatype-oid.html).
The first patch of the aboves contains doc patch which adds the
following note to html/datatype-oid.html. Does it make sense?
> Note: The OID alias types don't sctrictly comply the transaction
> isolation rules so do not use them where exact transaction
> isolation on the values of these types has a
> significance. Likewise, since they look as simple constants to
> planner so you might get slower plans than the queries joining
> the system tables correnspond to the OID types.
regards,
At Mon, 2 Mar 2015 17:50:37 -0600, Jim Nasby wrote
in <54f4f74d.8000...@bluetreble.com>
> On 3/2/15 3:56 PM, Andres Freund wrote:
> > On 2015-03-02 16:42:35 -0500, Robert Haas wrote:
> >> >On Tue, Feb 3, 2015 at 10:12 AM, Tom Lane wrote:
> >>> > >Two reasons this isn't terribly compelling are (1) it's creating a
> >>> > >join in a place where the planner can't possibly see it and optimize
> >>> > >it, and (2) you risk MVCC anomalies because the reg* output routines
> >>> > >would not be using the same snapshot as the calling query.
> >>> > >
> >>> > >We already have problem (2) with the existing reg* functions so I'm
> >>> > >not that excited about doubling down on the concept.
> >> >
> >> >I think I agree. I mean, I agree that this notation is more
> >> >convenient, but I don't really want to add a whole new slough of types
> >> >--- these will certainly not be the only ones we want once we go down
> >> >this path --- to the default install just for notational convenience.
> >> >It's arguable, of course, but I guess I'm going to vote against this
> >> >patch.
> > That's a justifyable position. I don't think there are other catalogs
> > referenced as pervasively in the catalog though.
> >
> > There's one additional point: Using reg* types in the catalog tables
> > themselves can make them*much* easier to read. I personally do look at
> > the catalogs a awful lot, and seing names instead of oids makes it
> > much
> > easier. And adding regtype/role would allow to cover nearly all types
> > containing oids.
>
> +1. Constantly joining catalog tables together is a royal PITA, and
> regnamespace is the biggest missing part of this (I typically don't
> look at roles too much, but I can certainly see it's importance).
>
> If we had more user friendly views on the catalogs maybe this wouldn't
> be an issue... but that's a much larger project.
>
> BTW, I think the potential for MVCC issues should be mentioned in the
> docs (http://www.postgresql.org/docs/devel/static/datatype-oid.html).
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From f6c9754f190d4cfe29f776ad1fe5f1a012533924 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Wed, 18 Feb 2015 14:38:32 +0900
Subject: [PATCH 1/2] Add regrole
Add new OID aliass type regrole. The new type has the scope of whole
the database cluster so it doesn't behave as the same as the existing
OID alias types which have database scope, concerning object
dependency. To get rid of confusion, inhibit constants of the new type
from appearing where dependencies are made involving it.
Documentation about this new type is added and some existing
descriptions are modified according to the restriction of the
type. Addition to it, put a note about possible MVCC violation and
optimization issues, which are general over the all reg* types.
---
doc/src/sgml/datatype.sgml| 55 +---
src/backend/bootstrap/bootstrap.c | 2 +
src/backend/catalog/dependency.c | 22
src/backend/utils/adt/regproc.c | 98 +++
src/backend/utils/adt/selfuncs.c | 2 +
src/backend/utils/cache/catcache.c| 1 +
src/include/catalog/pg_cast.h | 7 +++
src/include/catalog/pg_proc.h | 10
src/include/catalog/pg_type.h | 5 ++
src/include/utils/builtins.h | 5 ++
src/test/regress/expected/regproc.out | 26 +-
src/test/regress/sql/regproc.sql | 7 +++
12 files changed, 221 insertions(+), 19 deletions(-)
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index edf636b..dab5430 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -4319,8 +4319,9 @@ SET xmloption TO { DOCUMENT | CONTENT };
an object identifier. There are also several alias types for
oid: regproc, regprocedure,
regoper, regoperator, regclass,
-regtype, regconfig, and regdictionary.
- shows an overview.
+regtype, regrole, regconfig, and
+regdictionary. shows
+an overview.
@@ -4429,6 +4430,13 @@ SELECT * FROM pg_attribute
+regrole
+pg_authid
+role name
+smithee
+
[HACKERS] MD5 authentication needs help
It feels like MD5 has accumulated enough problems that we need to start looking for another way to store and pass passwords. The MD5 problems are: 1) MD5 makes users feel uneasy (though our usage is mostly safe) 2) The per-session salt sent to the client is only 32-bits, meaning that it is possible to reply an observed MD5 hash in ~16k connection attempts. 3) Using the user name for the MD5 storage salt allows the MD5 stored hash to be used on a different cluster if the user used the same password. 4) Using the user name for the MD5 storage salt causes the renaming of a user to break the stored password. For these reasons, it is probably time to start thinking about a replacement that fixes these issues. We would keep MD5 but recommend a better option. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On 03/03/2015 05:07 PM, Greg Stark wrote: > On Wed, Mar 4, 2015 at 12:17 AM, Jim Nasby wrote: >> I can make these changes if you want. > > Personally I'm just not convinced this is worth it. It makes the > catalogs harder for people to read and use and only benefits people > who have users named "all" or databases named "all", "sameuser", or > "samerole" which I can't really imagine would be anyone. > > If this were going to be the infrastructure on which lots of tools > rested rather than just a read-only view that was mostly going to be > read by humans that might be different. Are you envisioning a tool > that would look at this view, offer a gui for users to make changes > based on that information, and build a new pg_hba.conf to replace it? I'm planning to write such a tool. However, I am not concerned about weird name cases like the above. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 12:18 PM, Greg Stark wrote: > On Wed, Mar 4, 2015 at 12:34 AM, Haribabu Kommi > wrote: >> Out of curiosity, regarding the result materialize code addition, Any >> way the caller of "hba_settings" function >> "ExecMakeTableFunctionResult" also stores the results in tuple_store. >> Is there any advantage >> doing it in hba_settings function rather than in the caller? > > That might protect against concurrent pg_hba reloads, though I suspect > there's a CHECK_FOR_INTERRUPTS hanging out in that loop. We could > maybe put one in this loop and check if the parser memory context has > been reset. I feel there is no problem of current pg_hba reloads, because the check_for_interrupts doesn't reload the conf files. It will be done in the "postgresMain" function once the query finishes. Am I missing something? + foreach(line, parsed_hba_lines) In the above for loop it is better to add "check_for_interrupts" to avoid it looping if the parsed_hba_lines are more. > But the immediate problem is that having the API that looked up a line > by line number and then calling it repeatedly with successive line > numbers was O(n^2). Each time it was called it had to walk down the > list from the head all over again. 1 + 2 + 3 + 4 + ... n = n(n+1)/2 or > O(n^2). This isn't performance critical but it would not have run in a > reasonable length of time for large pg_hba files. > > And having to store the state between calls means the code is at least > as simple for the tuplestore, imho even simpler. Got it. Thanks. Regards, Hari Babu Fujitsu Australia -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Obsolete SnapshotNow reference within snapbuild.c
SnapBuildCommitTxn() has what I gather is an obsolete reference to SnapshotNow(). Attached patch corrects this. -- Peter Geoghegan diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index e911453..ff5ff26 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -1077,7 +1077,7 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid, /* refcount of the snapshot builder for the new snapshot */ SnapBuildSnapIncRefcount(builder->snapshot); - /* add a new SnapshotNow to all currently running transactions */ + /* add a new Snapshot to all currently running transactions */ SnapBuildDistributeNewCatalogSnapshot(builder, lsn); } else -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 12:34 AM, Haribabu Kommi wrote: > Out of curiosity, regarding the result materialize code addition, Any > way the caller of "hba_settings" function > "ExecMakeTableFunctionResult" also stores the results in tuple_store. > Is there any advantage > doing it in hba_settings function rather than in the caller? That might protect against concurrent pg_hba reloads, though I suspect there's a CHECK_FOR_INTERRUPTS hanging out in that loop. We could maybe put one in this loop and check if the parser memory context has been reset. But the immediate problem is that having the API that looked up a line by line number and then calling it repeatedly with successive line numbers was O(n^2). Each time it was called it had to walk down the list from the head all over again. 1 + 2 + 3 + 4 + ... n = n(n+1)/2 or O(n^2). This isn't performance critical but it would not have run in a reasonable length of time for large pg_hba files. And having to store the state between calls means the code is at least as simple for the tuplestore, imho even simpler. -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 12:17 AM, Jim Nasby wrote: > I can make these changes if you want. Personally I'm just not convinced this is worth it. It makes the catalogs harder for people to read and use and only benefits people who have users named "all" or databases named "all", "sameuser", or "samerole" which I can't really imagine would be anyone. If this were going to be the infrastructure on which lots of tools rested rather than just a read-only view that was mostly going to be read by humans that might be different. Are you envisioning a tool that would look at this view, offer a gui for users to make changes based on that information, and build a new pg_hba.conf to replace it? -- 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] Combining Aggregates
Robert Haas writes: > On Tue, Feb 24, 2015 at 2:20 PM, Peter Eisentraut wrote: >> I think the combine function is not actually a property of the >> aggregate, but a property of the transition function. If two aggregates >> have the same transition function, they will also have the same combine >> function. The combine function really just says, how do you combine two >> series of these function calls. Say maybe this should be put into >> pg_proc instead. (Or you make the transition functions transition >> operators and put the info into pg_operator instead, which is where >> function call optimization information is usually kept.) > This seems like a weird design to me. It's probably true that if the > transition function is the same, the state-combiner function will also > be the same. But the state-combiner function is only going to exist > for aggregate transition functions, not functions or operators in > general. So linking it from pg_proc or pg_operator feels wrong to me. FWIW, I agree with Robert. It's better to keep this in pg_aggregate. We don't need yet another mostly-empty column in pg_proc; and as for pg_operator, there is no expectation that an aggregate transition function is in pg_operator. Also, I don't think it's a foregone conclusion that same transition function implies same combiner function: that logic falls apart when you consider that one of them might be polymorphic and the other not. (Admittedly, that probably means the aggregate creator is missing a bet; but we should not design the catalog schema in a way that says that only maximally efficient aggregate designs are allowed.) 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] Proposal: knowing detail of config files via SQL
Jim, * Jim Nasby (jim.na...@bluetreble.com) wrote: > On 3/3/15 5:22 PM, Stephen Frost wrote: > >The > >problem with the role attribute approach is that they aren't inheirted > >the way GRANTs are, which means you can't have a "backup" role that is > >then granted out to users, you'd have to set a "BACKUP" role attribute > >for every role added. > > Yeah, but you'd still have to grant "backup" to every role created > anyway, right? Yes, you would. > Or you could create a role that has the backup attribute and then > grant that to users. Then they'd have to intentionally SET ROLE > my_backup_role to elevate their privilege. That seems like a safer > way to do things... This is already possible with the GRANT system- create a 'noinherit' role instead of an 'inherit' role. I agree it's safer to require a 'SET ROLE' and configure all of my systems with a noinherit 'admin' role. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] NULL-pointer check and incorrect comment for pstate in addRangeTableEntry
On Wed, Mar 4, 2015 at 6:43 AM, Robert Haas wrote: > That seems to make sense to me. Committed. Thanks. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Idea: closing the loop for "pg_ctl reload"
On 3/3/15 5:13 PM, Tom Lane wrote: Jim Nasby writes: On 3/3/15 11:48 AM, Andres Freund wrote: It'll be confusing to have different interfaces in one/multiple error cases. If we simply don't want the code complexity then fine, but I just don't buy this argument. How could it possibly be confusing? What I'm concerned about is confusing the code. There is a lot of stuff that looks at pidfiles and a lot of it is not very bright (note upthread argument about "cat" vs "head -1"). I don't want possibly localized (non-ASCII) text in there, especially when there's not going to be any sane way to know which encoding it's in. And I definitely don't want multiline error messages in there. Definitely no multi-line. If we keep that restriction, couldn't we just dedicate one entire line to the error message? ISTM that would be safe. It's possible we could dumb things down enough to meet these restrictions, but I'd really rather not go there at all. IMHO the added DBA convenience would be worth it (assuming we can make it safe). I know I'd certainly appreciate it... On 3/3/15 5:24 PM, Jan de Visser wrote:> On March 3, 2015 04:57:58 PM Jim Nasby wrote: >> On 3/3/15 11:48 AM, Andres Freund wrote: >>> I'm saying that you'll need a way to notice that a reload was processed >> > or not. And that can't really be the message itself, there has to be >> > some other field; like the timestamp Tom proposes. >> >> Ahh, right. We should make sure we don't go brain-dead if the system >> clock moves backwards. I assume we couldn't just fstat the file... > > The timestamp field is already there (in my patch). It gets populated when the > server starts and repopulated by SIGHUP_handler. It's the only timestamp > pg_ctl pays attention to. What happens if someone does a reload sometime in the future (perhaps because they've messed with the system clock for test purposes). Do we still detect a new reload? -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 5:57 AM, Greg Stark wrote: > On further review I've made a few more changes attached. > > I think we should change the column names to "users" and "databases" > to be clear they're lists and also to avoid the "user" SQL reserved > word. > > I removed the dependency on strlist_to_array which is in > objectaddress.c which isn't a very sensible dependency -- it does seem > like it would be handy to have a list-based version of construct_array > moved to arrayfuncs.c but for now it's not much more work to handle > these ourselves. > > I changed the options to accumulate one big array instead of an array > of bunches of options. Previously you could only end up with a > singleton array with a comma-delimited string or a two element array > with one of those and map=. The changes are fine, this eliminates the unnecessary dependency on objectaddress. > I think the error if pg_hba fails to reload needs to be LOG. It would > be too unexpected to the user who isn't necessarily the one who issued > the SIGHUP to spontaneously get a warning. > > I also removed the "mask" from local entries and made some of the > NULLS that shouldn't be possible to happen (unknown auth method or > connection method) actually throw errors. changes are fine. All the tests are passed. Out of curiosity, regarding the result materialize code addition, Any way the caller of "hba_settings" function "ExecMakeTableFunctionResult" also stores the results in tuple_store. Is there any advantage doing it in hba_settings function rather than in the caller? Regards, Hari Babu Fujitsu Australia -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On 3/3/15 5:22 PM, Stephen Frost wrote: The problem with the role attribute approach is that they aren't inheirted the way GRANTs are, which means you can't have a "backup" role that is then granted out to users, you'd have to set a "BACKUP" role attribute for every role added. Yeah, but you'd still have to grant "backup" to every role created anyway, right? Or you could create a role that has the backup attribute and then grant that to users. Then they'd have to intentionally SET ROLE my_backup_role to elevate their privilege. That seems like a safer way to do things... -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Providing catalog view to pg_hba.conf file - Patch submission
On 3/3/15 12:57 PM, Greg Stark wrote: On Tue, Mar 3, 2015 at 6:05 PM, Jim Nasby wrote: What about a separate column that's just the text from pg_hba? Or is that what you're opposed to? I'm not sure what you mean by that. There's a rawline field we could put somewhere but it contains the entire line. I mean have a field for each of user/databases that gives you valid pg_hba.conf output. That would allow for cut & paste. But perhaps that's just not a use case. FWIW, I'd say that having the individual array elements be correct is more important than what the result of array_out is. That way you could always do array_to_string(..., ', ') and get valid pg_hba output. Well I don't think you can get that without making the view less useful for every other purpose. Like, I would want to be able to do WHERE "user" @> array[?] or WHERE database = array[?] or to join against a list of users or databases somewhere else. I think we're screwed in that regard anyway, because of the special constructs. You'd need different logic to handle things like +role and sameuser. We might even end up painted in a corner where we can't change it in the future because it'll break everyone's scripts. To do what you suggest would mean the tokens will need to be quoted based on pg_hba.conf syntax requirements. That would mean I would need to check each variable or join value against pg_hba.conf's quoting requirements to compare with it. It seems more practical to have that knowledge if you're actually going to generate a pg_hba.conf than to pass around these quoted strings all the time. What about this: - database_specials enum[] contains all occurrences of all, sameuser, samerole and replication (or maybe it doesn't need to be an array) - in_roles name[] is a list of all cases of +role_name, with the + stripped off (I think the @ case is already handled before the SRF is called??) - role_specials enum[] handles all (enum[] for any future expansion) Alternatively in_roles could just be combined with role_specials as long as we preserve the +. Any tokens that match the special conditions do not show up in databases/users, and those fields become name[]. AFAIK that means the quoting should be identical (at least looking at check_db() and check_role()). I can make these changes if you want. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] failures with tuplesort and ordered set aggregates (due to 5cefbf5a6c44)
On Tue, Mar 3, 2015 at 3:53 PM, Robert Haas wrote: > I find your statement that this is a pre-existing issue in > tuplesort_begin_datum() to be pretty misleading, unless what you mean > by it is "pre-existing since November, when an earlier patch by Peter > Geoghegan changed the comments without changing the code to match". > Commit 5ea86e6e65dd2da3e9a3464484985d48328e7fe3, changed the comments > for the sortKey field to say that it would be set in all cases except > for the hash index case, but it did not make that statement true. Agreed. I believe that is in fact what I meant. I'm not trying to deflect blame - just to explain my understanding of the problem. > Subsequently, another of your patches, which I committed as > 5cefbf5a6c4466ac6b1cc2a4316b4eba9108c802, added a dependency on > state->sortKeys always being set. Now you've got this patch here, > which you claim makes sure that sortKeys is always set, but it > actually doesn't, which is why the above still crashes. There are not > so many tuplesort_begin_xxx routines that you couldn't audit them all > before submitting your patches. Sorry. I should have caught the hash index case too. > Anyway, I propose the attached instead, which makes both Tomas's test > case and the one above pass. My patch actually matches Andrew Gierth's datumsort patch, in that it also uses this convention, as I believe it should. For that reason, I'd prefer to make the comment added in November true, rather than changing the comment...I feel it ought to be true anyway (which is what I meant about a pre-existing issue). You'll still need the defenses you've added for the the hash case, of course. You might want to also put a comment specifying why it's necessary, since the hash tuple case is the oddball cases that doesn't always have "sortKeys" set. In other words, I suggest that you commit the union of our two patches, less your changes to the comments around "sortKeys'. Thanks -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Combining Aggregates
On Tue, Feb 24, 2015 at 2:20 PM, Peter Eisentraut wrote: > On 2/20/15 3:09 PM, Tomas Vondra wrote: >> The 'combine' function gets two such 'state' values, while transition >> gets 'state' + next value. > > I think the combine function is not actually a property of the > aggregate, but a property of the transition function. If two aggregates > have the same transition function, they will also have the same combine > function. The combine function really just says, how do you combine two > series of these function calls. Say maybe this should be put into > pg_proc instead. (Or you make the transition functions transition > operators and put the info into pg_operator instead, which is where > function call optimization information is usually kept.) This seems like a weird design to me. It's probably true that if the transition function is the same, the state-combiner function will also be the same. But the state-combiner function is only going to exist for aggregate transition functions, not functions or operators in general. So linking it from pg_proc or pg_operator feels wrong to me. -- 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] failures with tuplesort and ordered set aggregates (due to 5cefbf5a6c44)
On Fri, Feb 20, 2015 at 4:01 PM, Peter Geoghegan wrote: > On Fri, Feb 20, 2015 at 11:58 AM, Tomas Vondra > wrote: >> This seems to happen because ordered_set_startup() calls >> tuplesort_begin_datum() when (use_tuples == true), which only sets >> 'onlyKey' and leaves (sortKeys == NULL). So 'mergeruns' fails because it >> does not expect that. > > Oops. You're right. Attached patch fixes the issue, by making > tuplesort_begin_datum() consistent with other comparable routines for > other tuple cases. I think it makes sense to have the 'sortKeys' field > always set, and the 'onlyKey' field also set when that additional > optimization makes sense. That was what I understood the API to be, so > arguably this is a pre-existing issue with tuplesort_begin_datum(). This doesn't completely fix the issue. Even with this patch applied: CREATE TABLE stuff AS SELECT random()::text AS randtext FROM generate_series(1,5000); set maintenance_work_mem='1024'; create index on stuff using hash (randtext); ...wait for a bit, server crash... I find your statement that this is a pre-existing issue in tuplesort_begin_datum() to be pretty misleading, unless what you mean by it is "pre-existing since November, when an earlier patch by Peter Geoghegan changed the comments without changing the code to match". Commit 5ea86e6e65dd2da3e9a3464484985d48328e7fe3, changed the comments for the sortKey field to say that it would be set in all cases except for the hash index case, but it did not make that statement true. Subsequently, another of your patches, which I committed as 5cefbf5a6c4466ac6b1cc2a4316b4eba9108c802, added a dependency on state->sortKeys always being set. Now you've got this patch here, which you claim makes sure that sortKeys is always set, but it actually doesn't, which is why the above still crashes. There are not so many tuplesort_begin_xxx routines that you couldn't audit them all before submitting your patches. Anyway, I propose the attached instead, which makes both Tomas's test case and the one above pass. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company tuplesort-fixes.patch Description: binary/octet-stream -- 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] Idea: closing the loop for "pg_ctl reload"
On March 3, 2015 04:57:58 PM Jim Nasby wrote: > On 3/3/15 11:48 AM, Andres Freund wrote: > > I'm saying that you'll need a way to notice that a reload was processed > > or not. And that can't really be the message itself, there has to be > > some other field; like the timestamp Tom proposes. > > Ahh, right. We should make sure we don't go brain-dead if the system > clock moves backwards. I assume we couldn't just fstat the file... The timestamp field is already there (in my patch). It gets populated when the server starts and repopulated by SIGHUP_handler. It's the only timestamp pg_ctl pays attention to. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > The discussion I'm having with Peter on another thread is a very similar > > case that should be looping in, which is if we should continue to have > > any superuser check on updating catalog tables. He is advocating that > > we remove that also and let the superuser GRANT modification access on > > catalog tables to users. > > > For my part, I understood that we specifically didn't want to allow that > > for the same reason that we didn't want to simply depend on the GRANT > > system for the above functions, but everyone else on these discussions > > so far is advocating for using the GRANT system. My memory might be > > wrong, but I could have sworn that I had brought up exactly that > > suggestion in years past only to have it shot down. > > I'm a bit less comfortable with this one, mainly because the ability to > modify the system catalogs directly implies the ability to crash, corrupt, > or hijack the database in any number of non-obvious ways. I would go so > far as to argue that if you grant me modify permissions on many of the > catalogs, I would have the ability to become superuser outright, and > therefore any notion that such a grant is any weaker than granting full > superuserness is a dangerous lie. I tend to agree with this approach and it's what I was advocating for in my overall analysis of superuser checks that gave rise to the additional role attributes idea. If it can be used to become superuser then it doesn't make sense to have it be independent from superuser. > It may be that the same type of permissions escalation is possible with > some of the functions you mention above; but anywhere that it isn't, > I should think GRANT on the function is a reasonable solution. The functions identified for the new role attributes were specifically those that, I believe, could be used without also giving the user superuser rights. Those are: pg_start_backup, pg_stop_backup, pg_switch_xlog, pg_rotate_logfile, pg_create_restore_point, pg_xlog_replay_pause, lo_import, lo_export, and pg_xlog_replay_resume. > One aspect of this that merits some thought is that in some cases > access to some set of functions is best granted as a unit. That's > easy with role properties but much less so with plain GRANT. > Do we have enough such cases to make it an issue? That's what roles are for, in my mind. If we're fine with using the grant system to control executing these functions then I would suggest we address this by providing some pre-configured roles or even just documentation which list what a given role would need. That's certainly what a lot of people coming from other databases expect. The problem with the role attribute approach is that they aren't inheirted the way GRANTs are, which means you can't have a "backup" role that is then granted out to users, you'd have to set a "BACKUP" role attribute for every role added. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Idea: closing the loop for "pg_ctl reload"
Jim Nasby writes: > On 3/3/15 11:48 AM, Andres Freund wrote: >> It'll be confusing to have different interfaces in one/multiple error cases. > If we simply don't want the code complexity then fine, but I just don't > buy this argument. How could it possibly be confusing? What I'm concerned about is confusing the code. There is a lot of stuff that looks at pidfiles and a lot of it is not very bright (note upthread argument about "cat" vs "head -1"). I don't want possibly localized (non-ASCII) text in there, especially when there's not going to be any sane way to know which encoding it's in. And I definitely don't want multiline error messages in there. It's possible we could dumb things down enough to meet these restrictions, but I'd really rather not go there at all. 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] Idea: closing the loop for "pg_ctl reload"
On 3/3/15 11:48 AM, Andres Freund wrote: On 2015-03-03 11:43:46 -0600, Jim Nasby wrote: >It's certainly better than now, but why put DBAs through an extra step for >no reason? Because it makes it more complicated than it already is? It's nontrivial to capture the output, escape it to somehow fit into a delimited field, et al. I'd rather have a committed improvement, than talks about a bigger one. I don't see how this would be significantly more complex, but of course that's subjective. >Though, in the case of multiple errors perhaps it would be best >to just report a count and point them at the log. It'll be confusing to have different interfaces in one/multiple error cases. If we simply don't want the code complexity then fine, but I just don't buy this argument. How could it possibly be confusing? Either you'll get the actual error message or a message that says "Didn't work, check the log." And the error will always be in the log no matter what. I really can't imagine that anyone would be unhappy that we just up-front told them what the error was if we could. Why make them jump through needless hoops? > I'm saying that you'll need a way to notice that a reload was processed > or not. And that can't really be the message itself, there has to be > some other field; like the timestamp Tom proposes. Ahh, right. We should make sure we don't go brain-dead if the system clock moves backwards. I assume we couldn't just fstat the file... -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Proposal: knowing detail of config files via SQL
Stephen Frost writes: > It's not a documented policy but it's certainly what a whole slew of > functions *do*. Including pg_start_backup, pg_stop_backup, > pg_switch_xlog, pg_rotate_logfile, pg_create_restore_point, > pg_xlog_replay_pause, lo_import, lo_export, and pg_xlog_replay_resume, > pg_read_file, pg_read_text_file, pg_read_binary_file, and pg_ls_dir. > Indeed, it's the larger portion of what the additional role attributes > are for. I'm not entirely thrilled to hear that nearly all of that work > might be moot, but if that's the case then so be it and let's just > remove all those superuser checks and instead revoke EXECUTE from public > and let the superuser grant out those rights. It does seem like that could be an easier and more flexible answer than inventing a pile of role attributes. > The discussion I'm having with Peter on another thread is a very similar > case that should be looping in, which is if we should continue to have > any superuser check on updating catalog tables. He is advocating that > we remove that also and let the superuser GRANT modification access on > catalog tables to users. > For my part, I understood that we specifically didn't want to allow that > for the same reason that we didn't want to simply depend on the GRANT > system for the above functions, but everyone else on these discussions > so far is advocating for using the GRANT system. My memory might be > wrong, but I could have sworn that I had brought up exactly that > suggestion in years past only to have it shot down. I'm a bit less comfortable with this one, mainly because the ability to modify the system catalogs directly implies the ability to crash, corrupt, or hijack the database in any number of non-obvious ways. I would go so far as to argue that if you grant me modify permissions on many of the catalogs, I would have the ability to become superuser outright, and therefore any notion that such a grant is any weaker than granting full superuserness is a dangerous lie. It may be that the same type of permissions escalation is possible with some of the functions you mention above; but anywhere that it isn't, I should think GRANT on the function is a reasonable solution. One aspect of this that merits some thought is that in some cases access to some set of functions is best granted as a unit. That's easy with role properties but much less so with plain GRANT. Do we have enough such cases to make it an issue? 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] Proposal: knowing detail of config files via SQL
On Tue, Mar 3, 2015 at 10:29 PM, Stephen Frost wrote: >> -1. If that policy exists at all, it's a BAD policy, because it >> prevents users from changing the permissions using DDL. I think the >> superuser check should be inside the function, when, for example, it >> masks some of the output data depending on the user's permissions. >> But I see little virtue in handicapping an attempt by a superuser to >> grant SELECT rights on pg_file_settings. > > It's not a documented policy but it's certainly what a whole slew of > functions *do*. Including pg_start_backup, pg_stop_backup, > pg_switch_xlog, pg_rotate_logfile, pg_create_restore_point, > pg_xlog_replay_pause, lo_import, lo_export, and pg_xlog_replay_resume, > pg_read_file, pg_read_text_file, pg_read_binary_file, and pg_ls_dir. And the same issue comes up for the pg_hba_settings patch. Fwiw it's not entirely true that users can't change the permissions on these functions via DDL -- it's just a lot harder. They have to create a security-definer wrapper function and then grant access to that function to who they want. I'm generally of the opinion we should use the GRANT system and not hard-wire things but I also see the concern that it's really easy to grant access to something without realizing all the consequences. It's a lot harder to audit your system to be sure nothing inappropriate has been granted which can be escalated to super-user access. It's not clear how to determine what the set of things are that could do that. -- 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] Idea: GSoC - Query Rewrite with Materialized Views
On 3/3/15 3:34 PM, David Fetter wrote: On Tue, Mar 03, 2015 at 05:49:06PM -0300, Alvaro Herrera wrote: Jim Nasby wrote: FWIW, what I would find most useful at this point is a way to get the equivalent of an AFTER STATEMENT trigger that provided all changed rows in a MV as the result of a statement. Ah, like https://www.postgresql.org/message-id/1402790204.65037.YahooMailNeo%40web122301.mail.ne1.yahoo.com Yes, very much like that. Actually, I was talking about the next step beyond that. I don't want what changed in a single table; I want what changed *in the source of the entire MV*. Kevin has a whitepaper that describes how to do this in set notation; theoretically this is a matter of converting that to SQL. IIRC this needs the deltas and current (or maybe NEW and OLD) for every table in the MV. So one way you could model this is a function that accepts a bunch of NEW and OLD recordsets. Theoretically you could actually drive that with per-row triggers, but the performance would presumably suck. Next best thing would be providing NEW and OLD for AFTER STATEMENT triggers (what Kevin was talking about in that email). Though, if you're driving this at a statement level that means you can't actually reference the MV in a statement that's performing DML on any of the dependent tables. As you can see, this is all pretty involved. Doing just a small part of this would make for a good GSoC project. AFTER STATEMENT NEW and OLD might be a good project; I don't know how much more work Kevin's stuff needs. But there's much greater value in creating something that would take the definition for a MV and turn that into appropriate delta logic. That would be the basis for detecting if a MV was stale (beyond just the gross level check of were any of the tables involved touched), and is what is needed to do *any* kind of incremental update. That logic doesn't have to be driven by triggers. For example, you could have PgQ or similar capture all DML on all tables for a MV and feed that data to the delta logic on an async incremental basis. It's pretty easy for an end user to setup PgQ or similar but doing the delta logic is tightly coupled to the MV definition, which would be very hard for an end user to deal with. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Proposal: knowing detail of config files via SQL
* Robert Haas (robertmh...@gmail.com) wrote:
> On Sat, Feb 28, 2015 at 12:27 AM, Stephen Frost wrote:
> > While this generally "works", the usual expectation is that functions
> > that should be superuser-only have a check in the function rather than
> > depending on the execute privilege. I'm certainly happy to debate the
> > merits of that approach, but for the purposes of this patch, I'd suggest
> > you stick an if (!superuser()) ereport("must be superuser") into the
> > function itself and not work about setting the correct permissions on
> > it.
>
> -1. If that policy exists at all, it's a BAD policy, because it
> prevents users from changing the permissions using DDL. I think the
> superuser check should be inside the function, when, for example, it
> masks some of the output data depending on the user's permissions.
> But I see little virtue in handicapping an attempt by a superuser to
> grant SELECT rights on pg_file_settings.
It's not a documented policy but it's certainly what a whole slew of
functions *do*. Including pg_start_backup, pg_stop_backup,
pg_switch_xlog, pg_rotate_logfile, pg_create_restore_point,
pg_xlog_replay_pause, lo_import, lo_export, and pg_xlog_replay_resume,
pg_read_file, pg_read_text_file, pg_read_binary_file, and pg_ls_dir.
Indeed, it's the larger portion of what the additional role attributes
are for. I'm not entirely thrilled to hear that nearly all of that work
might be moot, but if that's the case then so be it and let's just
remove all those superuser checks and instead revoke EXECUTE from public
and let the superuser grant out those rights.
As for pg_stat_get_wal_senders, pg_stat_get_activity, and proably some
others, column-level privileges and/or RLS policies might be able to
provide what we want there (or possibly not), but it's something to
consider if we want to go this route.
For pg_signal_backend, we could revoke direct EXECUTE permissions on it
and then keep just the check that says only superusers can signal
superuser initiated processes, and then a superuser could grant EXECUTE
rights on it directly for users they want to have that access. We could
possibly also create new helper functions for pg_terminate and
pg_cancel.
The discussion I'm having with Peter on another thread is a very similar
case that should be looping in, which is if we should continue to have
any superuser check on updating catalog tables. He is advocating that
we remove that also and let the superuser GRANT modification access on
catalog tables to users.
For my part, I understood that we specifically didn't want to allow that
for the same reason that we didn't want to simply depend on the GRANT
system for the above functions, but everyone else on these discussions
so far is advocating for using the GRANT system. My memory might be
wrong, but I could have sworn that I had brought up exactly that
suggestion in years past only to have it shot down.
Thanks,
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] logical column ordering
On Tue, Mar 3, 2015 at 11:41:20AM -0600, Jim Nasby wrote: > >>Wouldn't it need attno info too, so all 3 orderings? > > > >Uh, what is the third ordering? Physical, logical, and ? It already > >gets information about dropped columns, if that is the third one. > > attnum; used in other catalogs to reference columns. > > If you're shuffling everything though pg_dump anyway then maybe it's > not needed... Yes, all those attno system table links are done with pg_dump SQL commands. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: raise default for max_wal_segments to 1GB
On 3/3/15 2:36 PM, Andrew Dunstan wrote: On 03/03/2015 02:59 PM, Josh Berkus wrote: On 03/03/2015 11:57 AM, Tom Lane wrote: Josh Berkus writes: Do we want to remove unit comments from all settings which accept "MB,GB" or "ms,s,min"? There's more than a few. I'd be in favor of this, but seems like (a) it should be universal, and (b) its own patch. Meh. Doing this strikes me as a serious documentation failure, since it is no longer apparent what happens if you put in a unitless number. Well, you know my opinion about documentation in the pg.conf file. However, I'm not confident I'm in the majority there. If we were consistent across all variables for each vartype we could maybe get away with this. But that's not the case. How are users supposed to know that statement_timeout is in ms while authentication_timeout is in seconds? I think there's a difference between comments about the function of a GUC and stating the units it's specified in. It's more than annoying to have to go and look that up where it's not stated. Look up the units? -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] remove pg_standby?
On Mon, Mar 2, 2015 at 6:22 PM, Fujii Masao wrote: > On Tue, Mar 3, 2015 at 3:07 AM, Josh Berkus wrote: >>> Per document, >>> >>> -- >>> In fast failover, the server is brought up immediately. Any WAL files >>> in the archive that have not yet been applied will be ignored, and all >>> transactions in those files are lost. To trigger a fast failover, >>> create a trigger file and write the word fast into it. pg_standby can >>> also be configured to execute a fast failover automatically if no new >>> WAL file appears within a defined interval. >>> -- >> >> I thought we had that as a 9.4 feature, actually. Well wait ... that's >> for streaming. > > s/9.4/9.3? > > That's different from one we had in 9.3. Fast failover that pg_standby > supports is something like the feature that I was proposing at > http://www.postgresql.org/message-id/cahgqgwhtvydqkzaywya9zyylecakif5p0kpcpune_tsrgtf...@mail.gmail.com > that is, the feature which allows us to give up replaying remaining > WAL data for some reasons at the standby promotion. OTOH, fast > failover that was supported in 9.3 enables us to skip an end-of-recovery > checkpoint at the promotion and reduce the failover time. Calling those things by the same name is very confusing. The data-losing feature ought to be called "immediate failover" or something else more dangerous-sounding than "fast". -- 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] Add more tests on event triggers
On Wed, Feb 25, 2015 at 10:03 AM, Fabien COELHO wrote: >> You can add tests in src/test/modules/dummy_seclabel. > > Patch attached to test sec label there, in addition to the other more > standard checks in event_trigger. These tests seem worthwhile to me. -- 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 in pg_dump
On 3/1/15 2:17 PM, Stephen Frost wrote: > Peter, if you have a minute, could you take a look at this thread and > discussion of having TAP tests under src/test/modules which need to > install an extension? I think it's something we certainly want to > support but I'm not sure it's a good idea to just run install in every > directory that has a prove_check. I don't think the way the tests are set up is scalable. Over time, we will surely want to test more and different extension dumping scenarios. We don't want to have to create a new fully built-out extension for each of those test cases, and we're going to run out of useful names for the extensions, too. Moreover, I would like to have tests of pg_dump in src/bin/pg_dump/, not in remote areas of the code. Here's what I would do: - set up basic scaffolding for TAP tests in src/bin/pg_dump - write a Perl function that can create an extension on the fly, given name, C code, SQL code - add to the proposed t/001_dump_test.pl code to write the extension - add that test to the pg_dump test suite Eventually, the dump-and-restore routine could also be refactored, but as long as we only have one test case, that can wait. -- 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] NULL-pointer check and incorrect comment for pstate in addRangeTableEntry
On Wed, Feb 25, 2015 at 7:27 AM, Michael Paquier wrote: > Coverity is pointing out that addRangeTableEntry contains the > following code path that does a NULL-pointer check on pstate: > if (pstate != NULL) > pstate->p_rtable = lappend(pstate->p_rtable, rte); > But pstate is dereferenced before in isLockedRefname when grabbing the > lock mode: > lockmode = isLockedRefname(pstate, refname) ? RowShareLock : AccessShareLock; > > Note that there is as well the following comment that is confusing on > top of addRangeTableEntry: > * If pstate is NULL, we just build an RTE and return it without adding it > * to an rtable list. > > So I have looked at all the code paths calling this function and found > first that the only potential point where pstate could be NULL is > transformTopLevelStmt in analyze.c. One level higher there are > parse_analyze_varparams and parse_analyze that may use pstate as NULL, > and even one level more higher in the stack there is > pg_analyze_and_rewrite. But well, looking at each case individually, > in all cases we never pass NULL for the parse tree node, so I think > that we should remove the comment on top of addRangeTableEntry, remove > the NULL-pointer check and add an assertion as in the patch attached. > > Thoughts? That seems to make sense to me. Committed. -- 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] Idea: GSoC - Query Rewrite with Materialized Views
On Tue, Mar 03, 2015 at 05:49:06PM -0300, Alvaro Herrera wrote: > Jim Nasby wrote: > > > FWIW, what I would find most useful at this point is a way to get > > the equivalent of an AFTER STATEMENT trigger that provided all > > changed rows in a MV as the result of a statement. > > Ah, like > https://www.postgresql.org/message-id/1402790204.65037.YahooMailNeo%40web122301.mail.ne1.yahoo.com Yes, very much like that. Kevin, might you be able to give some guidance on this? Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] CATUPDATE confusion?
On 2/28/15 6:32 PM, Stephen Frost wrote: > This isn't really /etc/shadow though, this is more like direct access to > the filesystem through the device node- and you'll note that Linux > certainly has got an independent set of permissions for that called the > capabilities system. That's because messing with those pieces can crash > the kernel. You're not going to crash the kernel if you goof up > /etc/shadow. I think this characterization is incorrect. The Linux capability system does not exist because the actions are scary or can crash the kernel. Capabilities exist because they are not attached to file system objects and can therefore not be represented using the usual permission system. Note that one can write directly to raw devices or the kernel memory through various /dev and /proc files. No "capability" protects against that. It's only the file permissions, possibly in combination with mount options. -- 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] Idea: GSoC - Query Rewrite with Materialized Views
Jim Nasby wrote: > FWIW, what I would find most useful at this point is a way to get the > equivalent of an AFTER STATEMENT trigger that provided all changed rows in a > MV as the result of a statement. Ah, like https://www.postgresql.org/message-id/1402790204.65037.YahooMailNeo%40web122301.mail.ne1.yahoo.com -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] add modulo (%) operator to pgbench
On Mon, Mar 2, 2015 at 5:41 PM, Fabien COELHO wrote: >> but I'd like to have a more robust discussion about what we want the error >> reporting to look like rather than just sliding it into this patch. > > As an input, I suggest that the error reporting feature should provide a > clue about where the issue is, including a line number and possibly the > offending line. Not sure what else is needed. I agree. But I think it might be better to try to put everything related to a single error on one line, in a consistent format, e.g.: bad.sql:3: syntax error in set command at column 25 -- 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: raise default for max_wal_segments to 1GB
On 03/03/2015 02:59 PM, Josh Berkus wrote: On 03/03/2015 11:57 AM, Tom Lane wrote: Josh Berkus writes: Do we want to remove unit comments from all settings which accept "MB,GB" or "ms,s,min"? There's more than a few. I'd be in favor of this, but seems like (a) it should be universal, and (b) its own patch. Meh. Doing this strikes me as a serious documentation failure, since it is no longer apparent what happens if you put in a unitless number. Well, you know my opinion about documentation in the pg.conf file. However, I'm not confident I'm in the majority there. I think there's a difference between comments about the function of a GUC and stating the units it's specified in. It's more than annoying to have to go and look that up where it's not stated. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Idea: GSoC - Query Rewrite with Materialized Views
> > I think even something that just does that in pure SQL/plpgsql would be a > big step forward, even if we wouldn't want it directly in the codebase. Something like creating a trigger under the hood each time a MV is created, that checks the changed rows on every statement against the query that generated the MV? That also seems feasible, but wouldn't it be rather too small for my three months of GSoC? 2015-03-02 20:22 GMT-03:00 Jim Nasby : > On 3/2/15 9:03 AM, Kevin Grittner wrote: > >> The query rewrite feature would be extremely desirable for us. >>> >Do you think that implementing the staleness check as suggested >>> >by Thomas could get us started in the query rewrite business? >>> >> There are many aspects related to the definition, maintenance, and >> use of MVs that need work; it seems to me that many of them can be >> pursued in parallel as long as people are communicating. Staleness >> tracking is definitely one aspect that is needed. If you want to >> put forward a proposal for that, which seems to be of a scope that >> is possible in the context of GSoC, that would be great. If there >> is any other aspect of the MV "big picture" that you can think of >> that you would like to tackle and seems of appropriate scope, >> please don't feel constrained to "staleness" as the only possible >> project; it was just one suggestion of something that might be of >> about the right size. >> > > FWIW, what I would find most useful at this point is a way to get the > equivalent of an AFTER STATEMENT trigger that provided all changed rows in > a MV as the result of a statement. That would at least allow people do do > their own MV refresh work without needing to study the methods for > identifying how the results of a statement impact what should be in the MV. > I think even something that just does that in pure SQL/plpgsql would be a > big step forward, even if we wouldn't want it directly in the codebase. > -- > Jim Nasby, Data Architect, Blue Treble Consulting > Data in Trouble? Get it in Treble! http://BlueTreble.com >
Re: [HACKERS] Patch: raise default for max_wal_segments to 1GB
No intention to hijack. Dropping issue for now. On Tue, Mar 3, 2015 at 2:05 PM, Josh Berkus wrote: > On 03/03/2015 10:58 AM, Corey Huinker wrote: > > Naive question: would it be /possible/ to change configuration to accept > > percentages, and have a percent mean "of existing RAM at startup time"? > > > > I ask because most of the tuning guidelines I see suggest setting memory > > parameters as a % of RAM available. > > Waaay off topic for this thread. Please don't hijack; start a new > thread if you want to discuss this. > > -- > Josh Berkus > PostgreSQL Experts Inc. > http://pgexperts.com >
Re: [HACKERS] Patch: raise default for max_wal_segments to 1GB
On 04/03/15 08:57, Tom Lane wrote: Josh Berkus writes: Do we want to remove unit comments from all settings which accept "MB,GB" or "ms,s,min"? There's more than a few. I'd be in favor of this, but seems like (a) it should be universal, and (b) its own patch. Meh. Doing this strikes me as a serious documentation failure, since it is no longer apparent what happens if you put in a unitless number. Now, if we were to change the server so that it *refused* settings that didn't have a unit, that argument would become moot. But I'm not going to defend the castle against the villagers who will show up if you do that. regards, tom lane How about introducing a 'strict' mode that does extra checking like that? JavaScript and Perl, both have a 'strict' mode. In strict mode you could insist on all quantities having units, and possibly some additional sanity checking - without enraging the peasants! Cheers, Gavin -- 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: raise default for max_wal_segments to 1GB
On 03/03/2015 11:57 AM, Tom Lane wrote: > Josh Berkus writes: >> Do we want to remove unit comments from all settings which accept >> "MB,GB" or "ms,s,min"? There's more than a few. I'd be in favor of >> this, but seems like (a) it should be universal, and (b) its own patch. > > Meh. Doing this strikes me as a serious documentation failure, since it > is no longer apparent what happens if you put in a unitless number. Well, you know my opinion about documentation in the pg.conf file. However, I'm not confident I'm in the majority there. > Now, if we were to change the server so that it *refused* settings that > didn't have a unit, that argument would become moot. But I'm not going > to defend the castle against the villagers who will show up if you do > that. No, thanks! -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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: raise default for max_wal_segments to 1GB
Josh Berkus writes: > Do we want to remove unit comments from all settings which accept > "MB,GB" or "ms,s,min"? There's more than a few. I'd be in favor of > this, but seems like (a) it should be universal, and (b) its own patch. Meh. Doing this strikes me as a serious documentation failure, since it is no longer apparent what happens if you put in a unitless number. Now, if we were to change the server so that it *refused* settings that didn't have a unit, that argument would become moot. But I'm not going to defend the castle against the villagers who will show up if you do that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql versus domains
2015-03-03 20:32 GMT+01:00 Tom Lane : > I wrote: > > Robert Haas writes: > >> On Thu, Feb 26, 2015 at 1:53 PM, Tom Lane wrote: > >>> To some extent this is a workaround for the fact that plpgsql does type > >>> conversions the way it does (ie, by I/O rather than by using the > parser's > >>> cast mechanisms). We've talked about changing that, but people seem to > >>> be afraid of the compatibility consequences, and I'm not planning to > fight > >>> two compatibility battles concurrently ;-) > > >> A sensible plan, but since we're here: > > >> - I do agree that changing the way PL/pgsql does those type > >> conversions is scary. I can't predict what will break, and there's no > >> getting around the scariness of that. > > >> - On the other hand, I do think it would be good to change it, because > >> this sucks: > > >> rhaas=# do $$ declare x int; begin x := (3.0::numeric)/(1.0::numeric); > end $$; > >> ERROR: invalid input syntax for integer: "3." > >> CONTEXT: PL/pgSQL function inline_code_block line 1 at assignment > > > If we did want to open that can of worms, my proposal would be to make > > plpgsql type conversions work like so: > > * If there is a cast pathway known to the core SQL parser, use that > > mechanism. > > * Otherwise, attempt to convert via I/O as we do today. > > This seems to minimize the risk of breaking things, although there would > > probably be corner cases that work differently (for instance I bet > boolean > > to text might turn out differently). In the very long run we could > perhaps > > deprecate and remove the second phase. > > Since people didn't seem to be running away screaming, here is a patch to > do that. I've gone through the list of existing casts, and as far as I > can tell the only change in behavior in cases that would have worked > before is the aforementioned boolean->string case. Before, if you made > plpgsql convert a bool to text, you got 't' or 'f', eg > > regression=# do $$declare t text; begin t := true; raise notice 't = %', > t; end $$; > NOTICE: t = t > DO > > Now you get 'true' or 'false', because that's what the cast does, eg > > regression=# do $$declare t text; begin t := true; raise notice 't = %', > t; end $$; > NOTICE: t = true > DO > > This seems to me to be a narrow enough behavioral change that we could > tolerate it in a major release. (Of course, maybe somebody out there > thinks that failures like the one Robert exhibits are a feature, in > which case they'd have a rather longer list of compatibility issues.) > > The performance gain is fairly nifty. I tested int->bigint coercions > like this: > > do $$ > declare b bigint; > begin > for i in 1 .. 100 loop > b := i; > end loop; > end$$; > > On my machine, in non-cassert builds, this takes around 750 ms in 9.4, > 610 ms in HEAD (so we already did something good, I'm not quite sure > what), and 420 ms with this patch. Another example is a no-op domain > conversion: > > create domain di int; > > do $$ > declare b di; > begin > for i in 1 .. 100 loop > b := i; > end loop; > end$$; > > This takes about 760 ms in 9.4, 660 ms in HEAD, 380 ms with patch. > > Comments? > it is perfect, thank you very much for this Regards Pavel Stehule > > 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] Patch: raise default for max_wal_segments to 1GB
On 03/03/2015 10:58 AM, Corey Huinker wrote: > Naive question: would it be /possible/ to change configuration to accept > percentages, and have a percent mean "of existing RAM at startup time"? > > I ask because most of the tuning guidelines I see suggest setting memory > parameters as a % of RAM available. Waaay off topic for this thread. Please don't hijack; start a new thread if you want to discuss this. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] plpgsql versus domains
I wrote:
> Robert Haas writes:
>> On Thu, Feb 26, 2015 at 1:53 PM, Tom Lane wrote:
>>> To some extent this is a workaround for the fact that plpgsql does type
>>> conversions the way it does (ie, by I/O rather than by using the parser's
>>> cast mechanisms). We've talked about changing that, but people seem to
>>> be afraid of the compatibility consequences, and I'm not planning to fight
>>> two compatibility battles concurrently ;-)
>> A sensible plan, but since we're here:
>> - I do agree that changing the way PL/pgsql does those type
>> conversions is scary. I can't predict what will break, and there's no
>> getting around the scariness of that.
>> - On the other hand, I do think it would be good to change it, because
>> this sucks:
>> rhaas=# do $$ declare x int; begin x := (3.0::numeric)/(1.0::numeric); end
>> $$;
>> ERROR: invalid input syntax for integer: "3."
>> CONTEXT: PL/pgSQL function inline_code_block line 1 at assignment
> If we did want to open that can of worms, my proposal would be to make
> plpgsql type conversions work like so:
> * If there is a cast pathway known to the core SQL parser, use that
> mechanism.
> * Otherwise, attempt to convert via I/O as we do today.
> This seems to minimize the risk of breaking things, although there would
> probably be corner cases that work differently (for instance I bet boolean
> to text might turn out differently). In the very long run we could perhaps
> deprecate and remove the second phase.
Since people didn't seem to be running away screaming, here is a patch to
do that. I've gone through the list of existing casts, and as far as I
can tell the only change in behavior in cases that would have worked
before is the aforementioned boolean->string case. Before, if you made
plpgsql convert a bool to text, you got 't' or 'f', eg
regression=# do $$declare t text; begin t := true; raise notice 't = %', t; end
$$;
NOTICE: t = t
DO
Now you get 'true' or 'false', because that's what the cast does, eg
regression=# do $$declare t text; begin t := true; raise notice 't = %', t; end
$$;
NOTICE: t = true
DO
This seems to me to be a narrow enough behavioral change that we could
tolerate it in a major release. (Of course, maybe somebody out there
thinks that failures like the one Robert exhibits are a feature, in
which case they'd have a rather longer list of compatibility issues.)
The performance gain is fairly nifty. I tested int->bigint coercions
like this:
do $$
declare b bigint;
begin
for i in 1 .. 100 loop
b := i;
end loop;
end$$;
On my machine, in non-cassert builds, this takes around 750 ms in 9.4,
610 ms in HEAD (so we already did something good, I'm not quite sure
what), and 420 ms with this patch. Another example is a no-op domain
conversion:
create domain di int;
do $$
declare b di;
begin
for i in 1 .. 100 loop
b := i;
end loop;
end$$;
This takes about 760 ms in 9.4, 660 ms in HEAD, 380 ms with patch.
Comments?
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index f364ce4..1621254 100644
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
*** do_compile(FunctionCallInfo fcinfo,
*** 559,566
{
function->fn_retbyval = typeStruct->typbyval;
function->fn_rettyplen = typeStruct->typlen;
- function->fn_rettypioparam = getTypeIOParam(typeTup);
- fmgr_info(typeStruct->typinput, &(function->fn_retinput));
/*
* install $0 reference, but only for polymorphic return
--- 559,564
*** plpgsql_compile_inline(char *proc_source
*** 803,809
char *func_name = "inline_code_block";
PLpgSQL_function *function;
ErrorContextCallback plerrcontext;
- Oid typinput;
PLpgSQL_variable *var;
int parse_rc;
MemoryContext func_cxt;
--- 801,806
*** plpgsql_compile_inline(char *proc_source
*** 876,883
/* a bit of hardwired knowledge about type VOID here */
function->fn_retbyval = true;
function->fn_rettyplen = sizeof(int32);
- getTypeInputInfo(VOIDOID, &typinput, &function->fn_rettypioparam);
- fmgr_info(typinput, &(function->fn_retinput));
/*
* Remember if function is STABLE/IMMUTABLE. XXX would it be better to
--- 873,878
*** build_datatype(HeapTuple typeTup, int32
*** 2200,2211
}
typ->typlen = typeStruct->typlen;
typ->typbyval = typeStruct->typbyval;
typ->typrelid = typeStruct->typrelid;
- typ->typioparam = getTypeIOParam(typeTup);
typ->collation = typeStruct->typcollation;
if (OidIsValid(collation) && OidIsValid(typ->collation))
typ->collation = collation;
- fmgr_info(typeStruct->typinput, &(typ->typinput));
typ->atttypmod = typmod;
return typ;
--- 2195,2205
}
typ->typlen = typeStruct->typlen;
typ->typbyval = typeStruct->typbyval;
+ typ->typisdomain = (typeStruct->typtype == TYPTYPE_DOMAIN);
typ->typrelid
Re: [HACKERS] Patch: raise default for max_wal_segments to 1GB
On 03/03/2015 02:12 AM, Fujii Masao wrote: > On Tue, Mar 3, 2015 at 8:51 AM, Josh Berkus wrote: >> On 03/02/2015 03:43 PM, Andres Freund wrote: >>> Hi, >>> >>> On 2015-03-02 15:40:27 -0800, Josh Berkus wrote: ! #max_wal_size = 1GB# in logfile segments >>> >>> Independent of the rest of the changes, the "in logfile segments" bit >>> should probably be changed. >> >> Point! Although I think it's fair to point out that that wasn't my >> omission, but Heikki's. >> >> Version C, with that correction, attached. > > Minor comments: > > "The default settings are 5 minutes and 128 MB, respectively." in wal.sgml > needs to be updated. > > > intmax_wal_size = 8;/* 128 MB */ > > It's better to update the above code in xlog.c. That's not essential, though. Attached is version D, which incorporates the above two changes, but NOT a general unit comment cleanup of postgresql.conf, which needs to be an entirely different patch. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml new file mode 100644 index 9261e7f..26214ec *** a/doc/src/sgml/config.sgml --- b/doc/src/sgml/config.sgml *** include_dir 'conf.d' *** 2406,2412 checkpoints. This is a soft limit; WAL size can exceed max_wal_size under special circumstances, like under heavy load, a failing archive_command, or a high ! wal_keep_segments setting. The default is 128 MB. Increasing this parameter can increase the amount of time needed for crash recovery. This parameter can only be set in the postgresql.conf --- 2406,2412 checkpoints. This is a soft limit; WAL size can exceed max_wal_size under special circumstances, like under heavy load, a failing archive_command, or a high ! wal_keep_segments setting. The default is 1 GB. Increasing this parameter can increase the amount of time needed for crash recovery. This parameter can only be set in the postgresql.conf diff --git a/doc/src/sgml/wal.sgml b/doc/src/sgml/wal.sgml new file mode 100644 index b57749f..f4083c3 *** a/doc/src/sgml/wal.sgml --- b/doc/src/sgml/wal.sgml *** *** 475,481 linkend="guc-checkpoint-timeout"> seconds, or if is about to be exceeded, whichever comes first. !The default settings are 5 minutes and 128 MB, respectively. If no WAL has been written since the previous checkpoint, new checkpoints will be skipped even if checkpoint_timeout has passed. (If WAL archiving is being used and you want to put a lower limit on how --- 475,481 linkend="guc-checkpoint-timeout"> seconds, or if is about to be exceeded, whichever comes first. !The default settings are 5 minutes and 1 GB, respectively. If no WAL has been written since the previous checkpoint, new checkpoints will be skipped even if checkpoint_timeout has passed. (If WAL archiving is being used and you want to put a lower limit on how diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c new file mode 100644 index a28155f..c3820fa *** a/src/backend/access/transam/xlog.c --- b/src/backend/access/transam/xlog.c *** extern uint32 bootstrap_data_checksum_ve *** 79,85 /* User-settable parameters */ ! int max_wal_size = 8; /* 128 MB */ int min_wal_size = 5; /* 80 MB */ int wal_keep_segments = 0; int XLOGbuffers = -1; --- 79,85 /* User-settable parameters */ ! int max_wal_size = 64; /* 1 GB */ int min_wal_size = 5; /* 80 MB */ int wal_keep_segments = 0; int XLOGbuffers = -1; diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c new file mode 100644 index d84dba7..cc4654a *** a/src/backend/utils/misc/guc.c --- b/src/backend/utils/misc/guc.c *** static struct config_int ConfigureNamesI *** 2171,2177 GUC_UNIT_XSEGS }, &max_wal_size, ! 8, 2, INT_MAX, NULL, assign_max_wal_size, NULL }, --- 2171,2177 GUC_UNIT_XSEGS }, &max_wal_size, ! 64, 2, INT_MAX, NULL, assign_max_wal_size, NULL }, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample new file mode 100644 index f8f9ce1..d3ef3e9 *** a/src/backend/utils/misc/postgresql.conf.sample --- b/src/backend/utils/misc/postgresql.conf.sample *** *** 198,204 # - Checkpoints - #checkpoint_timeout = 5min # range 30s-1h ! #max_wal_size = 128MB # in logfile segments #min_wal_size = 80MB #checkpoint_completion_target = 0.5 # checkpoint target duration, 0.0 - 1.0 #checkpoint_warning = 30s # 0 disables --- 198,204 # - Checkpoints - #checkpoint_timeout = 5min # range 30s-1h ! #max_wal_size = 1GB #min_wal_size = 80MB #checkpoint_completion_target = 0.5 # checkpoint t
Re: [HACKERS] Patch: raise default for max_wal_segments to 1GB
Naive question: would it be /possible/ to change configuration to accept percentages, and have a percent mean "of existing RAM at startup time"? I ask because most of the tuning guidelines I see suggest setting memory parameters as a % of RAM available. On Tue, Mar 3, 2015 at 1:29 PM, Heikki Linnakangas wrote: > On 03/03/2015 08:21 PM, Josh Berkus wrote: > >> On 03/03/2015 10:15 AM, Josh Berkus wrote: >> >>> On 03/02/2015 11:25 PM, Heikki Linnakangas wrote: >>> I propose that we remove the comment from max_wal_size, and also remove the "in milliseconds" from wal_receiver_timeout and autovacuum_vacuum_cost_delay. >>> >>> +1 >>> >>> >> Actually, let's be consistent about this. It makes no sense to remove >> unit comments from some settings which accept ms but not others. >> >> Do we want to remove unit comments from all settings which accept >> "MB,GB" or "ms,s,min"? There's more than a few. I'd be in favor of >> this, but seems like (a) it should be universal, and (b) its own patch. >> > > I think it's a good rule that if the commented-out default in the sample > file does not contain a unit, then the base unit is in the comment. > Otherwise it's not. For example: > > #shared_buffers = 32MB # min 128kB > # (change requires restart) > > The base unit is BLCKSZ, i.e. 8k, but usually people will usually use > MB/GB. And that is evident from the default value, 32MB, so there's no need > to mention it in the comment. > > #tcp_keepalives_idle = 0# TCP_KEEPIDLE, in seconds; > # 0 selects the system default > > Here it's not obvious what the unit should be from the default itself. So > the comment says it's "in seconds". > > - Heikki > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Tue, Mar 3, 2015 at 6:05 PM, Jim Nasby wrote: > What about a separate column that's just the text from pg_hba? Or is that > what you're opposed to? I'm not sure what you mean by that. There's a rawline field we could put somewhere but it contains the entire line. > FWIW, I'd say that having the individual array elements be correct is more > important than what the result of array_out is. That way you could always do > array_to_string(..., ', ') and get valid pg_hba output. Well I don't think you can get that without making the view less useful for every other purpose. Like, I would want to be able to do WHERE "user" @> array[?] or WHERE database = array[?] or to join against a list of users or databases somewhere else. To do what you suggest would mean the tokens will need to be quoted based on pg_hba.conf syntax requirements. That would mean I would need to check each variable or join value against pg_hba.conf's quoting requirements to compare with it. It seems more practical to have that knowledge if you're actually going to generate a pg_hba.conf than to pass around these quoted strings all the time. On further review I've made a few more changes attached. I think we should change the column names to "users" and "databases" to be clear they're lists and also to avoid the "user" SQL reserved word. I removed the dependency on strlist_to_array which is in objectaddress.c which isn't a very sensible dependency -- it does seem like it would be handy to have a list-based version of construct_array moved to arrayfuncs.c but for now it's not much more work to handle these ourselves. I changed the options to accumulate one big array instead of an array of bunches of options. Previously you could only end up with a singleton array with a comma-delimited string or a two element array with one of those and map=. I think the error if pg_hba fails to reload needs to be LOG. It would be too unexpected to the user who isn't necessarily the one who issued the SIGHUP to spontaneously get a warning. I also removed the "mask" from local entries and made some of the NULLS that shouldn't be possible to happen (unknown auth method or connection method) actually throw errors. -- greg *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml *** *** 7393,7398 --- 7393,7403 views + + pg_hba_settings + client authentication settings + + *** *** 9696,9699 SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx --- 9701,9786 + + pg_hba_settings + + +pg_hba_settings + + + +The read-only pg_hba_settings view provides +access to the client authentication configuration from pg_hba.conf. +Access to this view is limited to superusers. + + + +pg_hba_settings Columns + + + + + Name + Type + Description + + + + + type + text + Type of connection + + + databases + text[] + List of database names + + + users + text[] + List of user names + + + address + inet + Client machine address + + + mask + inet + IP Mask + + + compare_method + text + IP address comparison method + + + hostname + text + Client host name + + + method + text + Authentication method + + + options + text[] + Configuration options set for authentication method + + + line_number + integer + +Line number within client authentication configuration file +the current value was set at + + + + + + *** a/doc/src/sgml/client-auth.sgml --- b/doc/src/sgml/client-auth.sgml *** *** 680,685 local all @admins,+supportmd5 --- 680,690 local db1,db2,@demodbs all md5 + + + The contents of this file are reflected in the pg_hba_settings view. + See for details. + *** a/src/backend/catalog/system_views.sql --- b/src/backend/catalog/system_views.sql *** *** 414,419 CREATE RULE pg_settings_n AS --- 414,424 GRANT SELECT, UPDATE ON pg_settings TO PUBLIC; + CREATE VIEW pg_hba_settings AS + SELECT * FROM pg_hba_settings() AS A; + + REVOKE ALL on pg_hba_settings FROM public; + CREATE VIEW pg_timezone_abbrevs AS SELECT * FROM pg_timezone_abbrevs(); *** a/src/backend/libpq/hba.c --- b/src/backend/libpq/hba.c *** *** 25,38 --- 25,43 #include #include + #include "access/htup_details.h" #include "catalog/pg_collation.h" + #include "catalog/pg_type.h" + #include "funcapi.h" #include "libpq/ip.h" #include "libpq/libpq.h" + #i
Re: [HACKERS] Normalizing units for postgresql.conf WAS: Patch: raise default for max_wal_segments to 1GB
On 03/03/2015 10:37 AM, Heikki Linnakangas wrote: > On 03/03/2015 08:31 PM, Josh Berkus wrote: >> On 03/03/2015 10:29 AM, Heikki Linnakangas wrote: >>> On 03/03/2015 08:21 PM, Josh Berkus wrote: On 03/03/2015 10:15 AM, Josh Berkus wrote: > On 03/02/2015 11:25 PM, Heikki Linnakangas wrote: >> I propose that we remove the comment from max_wal_size, and also >> remove >> the "in milliseconds" from wal_receiver_timeout and >> autovacuum_vacuum_cost_delay. > > +1 > Actually, let's be consistent about this. It makes no sense to remove unit comments from some settings which accept ms but not others. Do we want to remove unit comments from all settings which accept "MB,GB" or "ms,s,min"? There's more than a few. I'd be in favor of this, but seems like (a) it should be universal, and (b) its own patch. >>> >>> I think it's a good rule that if the commented-out default in the sample >>> file does not contain a unit, then the base unit is in the comment. >>> Otherwise it's not. For example: >>> >>> #shared_buffers = 32MB# min 128kB >>> # (change requires restart) >>> >>> The base unit is BLCKSZ, i.e. 8k, but usually people will usually use >>> MB/GB. And that is evident from the default value, 32MB, so there's no >>> need to mention it in the comment. >>> >>> #tcp_keepalives_idle = 0# TCP_KEEPIDLE, in seconds; >>> # 0 selects the system default >>> >>> Here it's not obvious what the unit should be from the default itself. >>> So the comment says it's "in seconds". >> >> Sure. Although, do we take (s) for tcp_keepalives_idle? Or only an INT? > > It's a "time unit", so you can say "10s" or "1ms". If you don't > specify a unit, it implies seconds. So if we're going to make this consistent, let's make it consistent. 1. All GUCs which accept time/size units will have them on the default setting. 2. Time/size comments will be removed, *except* from GUCs which do not accept (ms/s/min) or (kB/MB/GB). Argument for: the current inconsistency confuses new users and his entirely historical in nature. Argument Against: will create unnecessary diff changes between 9.4's pg.conf and 9.5's pg.conf. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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: raise default for max_wal_segments to 1GB
On 03/03/2015 08:31 PM, Josh Berkus wrote: On 03/03/2015 10:29 AM, Heikki Linnakangas wrote: On 03/03/2015 08:21 PM, Josh Berkus wrote: On 03/03/2015 10:15 AM, Josh Berkus wrote: On 03/02/2015 11:25 PM, Heikki Linnakangas wrote: I propose that we remove the comment from max_wal_size, and also remove the "in milliseconds" from wal_receiver_timeout and autovacuum_vacuum_cost_delay. +1 Actually, let's be consistent about this. It makes no sense to remove unit comments from some settings which accept ms but not others. Do we want to remove unit comments from all settings which accept "MB,GB" or "ms,s,min"? There's more than a few. I'd be in favor of this, but seems like (a) it should be universal, and (b) its own patch. I think it's a good rule that if the commented-out default in the sample file does not contain a unit, then the base unit is in the comment. Otherwise it's not. For example: #shared_buffers = 32MB# min 128kB # (change requires restart) The base unit is BLCKSZ, i.e. 8k, but usually people will usually use MB/GB. And that is evident from the default value, 32MB, so there's no need to mention it in the comment. #tcp_keepalives_idle = 0# TCP_KEEPIDLE, in seconds; # 0 selects the system default Here it's not obvious what the unit should be from the default itself. So the comment says it's "in seconds". Sure. Although, do we take (s) for tcp_keepalives_idle? Or only an INT? It's a "time unit", so you can say "10s" or "1ms". If you don't specify a unit, it implies seconds. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: raise default for max_wal_segments to 1GB
On 03/03/2015 10:29 AM, Heikki Linnakangas wrote: > On 03/03/2015 08:21 PM, Josh Berkus wrote: >> On 03/03/2015 10:15 AM, Josh Berkus wrote: >>> On 03/02/2015 11:25 PM, Heikki Linnakangas wrote: I propose that we remove the comment from max_wal_size, and also remove the "in milliseconds" from wal_receiver_timeout and autovacuum_vacuum_cost_delay. >>> >>> +1 >>> >> >> Actually, let's be consistent about this. It makes no sense to remove >> unit comments from some settings which accept ms but not others. >> >> Do we want to remove unit comments from all settings which accept >> "MB,GB" or "ms,s,min"? There's more than a few. I'd be in favor of >> this, but seems like (a) it should be universal, and (b) its own patch. > > I think it's a good rule that if the commented-out default in the sample > file does not contain a unit, then the base unit is in the comment. > Otherwise it's not. For example: > > #shared_buffers = 32MB# min 128kB > # (change requires restart) > > The base unit is BLCKSZ, i.e. 8k, but usually people will usually use > MB/GB. And that is evident from the default value, 32MB, so there's no > need to mention it in the comment. > > #tcp_keepalives_idle = 0# TCP_KEEPIDLE, in seconds; > # 0 selects the system default > > Here it's not obvious what the unit should be from the default itself. > So the comment says it's "in seconds". Sure. Although, do we take (s) for tcp_keepalives_idle? Or only an INT? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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: raise default for max_wal_segments to 1GB
On 03/03/2015 08:21 PM, Josh Berkus wrote: On 03/03/2015 10:15 AM, Josh Berkus wrote: On 03/02/2015 11:25 PM, Heikki Linnakangas wrote: I propose that we remove the comment from max_wal_size, and also remove the "in milliseconds" from wal_receiver_timeout and autovacuum_vacuum_cost_delay. +1 Actually, let's be consistent about this. It makes no sense to remove unit comments from some settings which accept ms but not others. Do we want to remove unit comments from all settings which accept "MB,GB" or "ms,s,min"? There's more than a few. I'd be in favor of this, but seems like (a) it should be universal, and (b) its own patch. I think it's a good rule that if the commented-out default in the sample file does not contain a unit, then the base unit is in the comment. Otherwise it's not. For example: #shared_buffers = 32MB # min 128kB # (change requires restart) The base unit is BLCKSZ, i.e. 8k, but usually people will usually use MB/GB. And that is evident from the default value, 32MB, so there's no need to mention it in the comment. #tcp_keepalives_idle = 0# TCP_KEEPIDLE, in seconds; # 0 selects the system default Here it's not obvious what the unit should be from the default itself. So the comment says it's "in seconds". - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Comparing primary/HS standby in tests
On 03/03/2015 07:49 AM, Andres Freund wrote: > I'd very much like to add a automated test like this to the tree, but I > don't see wa way to do that sanely without a comparison tool... We could use a comparison tool anyway. Baron Schwartz was pointing out that Percona has a comparison tool for MySQL, and the amount of "drift" and corruption that they find in a large replication cluster is generally pretty alarming, and *always* present. While our replication isn't as flaky as MySQL's, networks are often lossy or corrupt. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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: raise default for max_wal_segments to 1GB
On 03/03/2015 10:15 AM, Josh Berkus wrote: > On 03/02/2015 11:25 PM, Heikki Linnakangas wrote: >> I propose that we remove the comment from max_wal_size, and also remove >> the "in milliseconds" from wal_receiver_timeout and >> autovacuum_vacuum_cost_delay. > > +1 > Actually, let's be consistent about this. It makes no sense to remove unit comments from some settings which accept ms but not others. Do we want to remove unit comments from all settings which accept "MB,GB" or "ms,s,min"? There's more than a few. I'd be in favor of this, but seems like (a) it should be universal, and (b) its own patch. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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: raise default for max_wal_segments to 1GB
On 03/02/2015 11:25 PM, Heikki Linnakangas wrote: > I propose that we remove the comment from max_wal_size, and also remove > the "in milliseconds" from wal_receiver_timeout and > autovacuum_vacuum_cost_delay. +1 -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] Providing catalog view to pg_hba.conf file - Patch submission
On 3/3/15 9:08 AM, Greg Stark wrote:
On Mon, Jun 30, 2014 at 8:06 AM, Abhijit Menon-Sen wrote:
After sleeping on it, I realised that the code would return '{all}' for
'all' in pg_hba.conf, but '{"all"}' for '"all"'. So it's not exactly
ambiguous, but I don't think it's especially useful for callers.
Hm. Nope, it doesn't. It just says {all} regardless of whether "all"
is quoted or not.
This makes sense, the rules for when to quote things in pg_hba and
when to quote things for arrays are separate. And we definitely don't
want to start adding quotes to every token that the parser noted was
quoted because then you'll start seeing things like {"\"all\""} and
{"\"database with space\""} which won't make it any easier to match
things.
I'm not sure adding a separate column is really the solution either.
You can have things like all,databasename (which is the keyword "all"
not a database "all). Or more likely something like sameuser,bob or
even things like replication,all.
What about a separate column that's just the text from pg_hba? Or is
that what you're opposed to?
FWIW, I'd say that having the individual array elements be correct is
more important than what the result of array_out is. That way you could
always do array_to_string(..., ', ') and get valid pg_hba output.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.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] Abbreviated keys for text cost model fix
On Tue, Mar 3, 2015 at 2:00 AM, Jeremy Harris wrote: > Yes; there seemed no advantage to any additional complexity. > The merge consistently performs fewer comparisons than the > quicksort, on random input - and many fewer if there are > any sorted (or reverse-sorted) sections. However, it also > consistently takes slightly longer (a few percent). I was > unable to chase this down but assume it to be a cacheing > effect. So I don't currently think it should replace the > current sort for all use. It's definitely caching effects. That's a very important consideration here. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Partitioning WIP patch
On Fri, Feb 27, 2015 at 09:09:35AM +0900, Amit Langote wrote: > On 27-02-2015 AM 03:24, Andres Freund wrote: > > On 2015-02-26 12:15:17 +0900, Amit Langote wrote: > >> On 26-02-2015 AM 05:15, Josh Berkus wrote: > >>> I would love to have it for 9.5, but I guess the > >>> patch isn't nearly baked enough for that? > > > >> I'm not quite sure what would qualify as baked enough for 9.5 though we > >> can surely try to reach some consensus on various implementation aspects > >> and perhaps even get it ready in time for 9.5. > > > > I think it's absolutely unrealistic to get this into 9.5. There's barely > > been any progress on the current (last!) commitfest - where on earth > > should the energy come to make this patch ready? And why would that be > > fair against all the others that have submitted in time? > > > > I realize and I apologize that it was irresponsible of me to have said > that; maybe got a bit too excited. I do not want to unduly draw people's > time on something that's not quite ready while there are other things > people have worked hard on to get in time. In all earnestness, I say we > spend time perfecting those things. > > I'll add this into CF-JUN'15. I will keep posting updates meanwhile so > that when that commitfest finally starts, we will have something worth > considering. I am _very_ glad you have started on this. There is a huge need for this, and I am certainly excited about it. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Idea: closing the loop for "pg_ctl reload"
On 2015-03-03 11:43:46 -0600, Jim Nasby wrote: > It's certainly better than now, but why put DBAs through an extra step for > no reason? Because it makes it more complicated than it already is? It's nontrivial to capture the output, escape it to somehow fit into a delimited field, et al. I'd rather have a committed improvement, than talks about a bigger one. > Though, in the case of multiple errors perhaps it would be best > to just report a count and point them at the log. It'll be confusing to have different interfaces in one/multiple error cases. > >Generally we obviously need some status to indicate that the config file > >has been reloaded, but that could be easily combined with storing the > >error message. > > Not sure I'm following... are you saying we should include the error message > in postmaster.pid? I'm saying that you'll need a way to notice that a reload was processed or not. And that can't really be the message itself, there has to be some other field; like the timestamp Tom proposes. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Idea: closing the loop for "pg_ctl reload"
On 3/3/15 11:33 AM, Andres Freund wrote: On 2015-03-03 11:09:29 -0600, Jim Nasby wrote: On 3/3/15 9:26 AM, Andres Freund wrote: On 2015-03-03 15:21:24 +, Greg Stark wrote: Fwiw this concerns me slightly. I'm sure a lot of people are doing things like "kill -HUP `cat .../postmaster.pid`" or the equivalent. postmaster.pid already contains considerably more than just the pid. e.g. 4071 /srv/dev/pgdev-master 1425396089 5440 /tmp localhost 5440001 82345992 If we already have all this extra stuff, why not include an actual error message then, or at least the first line of an error (or maybe just swap any newlines with spaces)? It's often several errors and such. I think knowing that it failed and that you should look into the error log is already quite helpful already. It's certainly better than now, but why put DBAs through an extra step for no reason? Though, in the case of multiple errors perhaps it would be best to just report a count and point them at the log. Generally we obviously need some status to indicate that the config file has been reloaded, but that could be easily combined with storing the error message. Not sure I'm following... are you saying we should include the error message in postmaster.pid? -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] logical column ordering
On 3/3/15 11:26 AM, Bruce Momjian wrote: On Tue, Mar 3, 2015 at 11:24:38AM -0600, Jim Nasby wrote: On 3/3/15 11:23 AM, Bruce Momjian wrote: On Thu, Feb 26, 2015 at 01:55:44PM -0800, Josh Berkus wrote: On 02/26/2015 01:54 PM, Alvaro Herrera wrote: This patch decouples these three things so that they can changed freely -- but provides no user interface to do so. I think that trying to only decouple the thing we currently have in two pieces, and then have a subsequent patch to decouple again, is additional conceptual complexity for no gain. Oh, I didn't realize there weren't commands to change the LCO. Without at least SQL syntax for LCO, I don't see why we'd take it; this sounds more like a WIP patch. FYI, pg_upgrade is going to need pg_dump --binary-upgrade to output the columns in physical order with some logical ordering information, i.e. pg_upgrade cannot be passed only logical ordering from pg_dump. Wouldn't it need attno info too, so all 3 orderings? Uh, what is the third ordering? Physical, logical, and ? It already gets information about dropped columns, if that is the third one. attnum; used in other catalogs to reference columns. If you're shuffling everything though pg_dump anyway then maybe it's not needed... -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] deparsing utility commands
Stephen,
Thanks for the review. I have pushed these patches, squashed in one
commit, with the exception of the one that changed ALTER TABLE. You had
enough comments about that one that I decided to put it aside for now,
and get the other ones done. I will post a new series shortly.
I fixed (most of?) the issues you pointed out; some additional comments:
Stephen Frost wrote:
> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> > @@ -1004,6 +1004,10 @@ AddNewRelationType(const char *typeName,
> > * use_user_acl: TRUE if should look for user-defined default permissions;
> > * if FALSE, relacl is always set NULL
> > * allow_system_table_mods: TRUE to allow creation in system namespaces
> > + * is_internal: is this a system-generated catalog?
>
> Shouldn't adding the comment for is_internal be done independently?
> It's clearly missing in master and fixing that hasn't got anything to do
> with the other changes happening here.
That was pushed separately and backpatched to 9.3. Turns out it was my
own very old mistake.
> This patch did make me think that there's quite a few places which could
> use ObjectAddressSet() that aren't (even after this patch). I don't
> think we need to stress over that, but it might be nice to mark that
> down as a TODO for someone to go through all the places where we
> construct an ObjectAddress and update them to use the new macro.
Agreed. I fixed the ones in these patches, but of course there's a lot
of them in other places.
> > @@ -772,7 +775,7 @@ DefineOpFamily(CreateOpFamilyStmt *stmt)
> > * other commands called ALTER OPERATOR FAMILY exist, but go through
> > * different code paths.
> > */
> > -Oid
> > +void
> > AlterOpFamily(AlterOpFamilyStmt *stmt)
> > {
> > Oid amoid, /* our AM's oid */
> > @@ -826,8 +829,6 @@ AlterOpFamily(AlterOpFamilyStmt *stmt)
> > AlterOpFamilyAdd(stmt->opfamilyname, amoid, opfamilyoid,
> > maxOpNumber, maxProcNumber,
> > stmt->items);
> > -
> > - return opfamilyoid;
> > }
>
> This feels a bit funny to me..? Why not construct and return an
> ObjectAddress like the other Alter functions?
The reason for not changing AlterOpFamily is that it needs completely
separate support for event triggers -- a simple ObjectAddress is not
enough. If you had a look at the support for GrantStmt in the rest of
the series, you have an idea of what supporting this one needs: we need
to store additional information somewhere in the event trigger queue.
Most likely, this function will end up returning an ObjectAddress as
well, but I want to write the deparse code first to make sure the change
will make sense.
In the committed patch, I ended up not changing this from Oid to void;
that was just pointless churn.
> > --- a/src/backend/commands/extension.c
> > +++ b/src/backend/commands/extension.c
> > @@ -2402,7 +2402,7 @@ extension_config_remove(Oid extensionoid, Oid
> > tableoid)
> > * Execute ALTER EXTENSION SET SCHEMA
> > */
> > ObjectAddress
> > -AlterExtensionNamespace(List *names, const char *newschema)
> > +AlterExtensionNamespace(List *names, const char *newschema, Oid *oldschema)
> > {
> > char *extensionName;
> > Oid extensionOid;
>
> Output parameter is not an ObjectAddress for this one? The other case
> where a schema was an output parameter, it was.. Feels a little odd.
What's going on here is that this function is not called directly by
ProcessUtilitySlow, but rather through ExecAlterObjectSchemaStmt(); it
seemed simpler to keep the OID return here, and only build the
ObjectAddress when needed. I did it that way because there's a path
through AlterExtensionNamespace itself that goes back through
AlterObjectNamespace_oid that requires the OID, and the address would
just be clutter most of the time. It seemed simpler. Not that much of
an issue anyhow, since this only affects four functions or so; it's
easily changed if necessary.
> Also, the function comment needs updating to reflect the new output
> parameter.
A large number of these functions were not documenting their APIs at
all, and most of them seemed self-explanatory. I updated the ones for
which it seemed worthwhile.
> Not sure if it's most helpful for me to continue to provide reviews or
> if it'd be useful for me to help with actually making some of these
> changes- please let me know your thoughts and I'll do my best to help.
Given the number of patches in the series, I think it's easiest for both
you and for me to give comments. I rebase this stuff pretty frequently
for submission.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-h
Re: [HACKERS] Idea: closing the loop for "pg_ctl reload"
On 2015-03-03 11:09:29 -0600, Jim Nasby wrote: > On 3/3/15 9:26 AM, Andres Freund wrote: > >On 2015-03-03 15:21:24 +, Greg Stark wrote: > >>Fwiw this concerns me slightly. I'm sure a lot of people are doing > >>things like "kill -HUP `cat .../postmaster.pid`" or the equivalent. > > > >postmaster.pid already contains considerably more than just the pid. e.g. > >4071 > >/srv/dev/pgdev-master > >1425396089 > >5440 > >/tmp > >localhost > > 5440001 82345992 > > If we already have all this extra stuff, why not include an actual error > message then, or at least the first line of an error (or maybe just swap any > newlines with spaces)? It's often several errors and such. I think knowing that it failed and that you should look into the error log is already quite helpful already. Generally we obviously need some status to indicate that the config file has been reloaded, but that could be easily combined with storing the error message. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] logical column ordering
On Tue, Mar 3, 2015 at 11:24:38AM -0600, Jim Nasby wrote: > On 3/3/15 11:23 AM, Bruce Momjian wrote: > >On Thu, Feb 26, 2015 at 01:55:44PM -0800, Josh Berkus wrote: > >>On 02/26/2015 01:54 PM, Alvaro Herrera wrote: > >>>This patch decouples these three things so that they > >>>can changed freely -- but provides no user interface to do so. I think > >>>that trying to only decouple the thing we currently have in two pieces, > >>>and then have a subsequent patch to decouple again, is additional > >>>conceptual complexity for no gain. > >> > >>Oh, I didn't realize there weren't commands to change the LCO. Without > >>at least SQL syntax for LCO, I don't see why we'd take it; this sounds > >>more like a WIP patch. > > > >FYI, pg_upgrade is going to need pg_dump --binary-upgrade to output the > >columns in physical order with some logical ordering information, i.e. > >pg_upgrade cannot be passed only logical ordering from pg_dump. > > Wouldn't it need attno info too, so all 3 orderings? Uh, what is the third ordering? Physical, logical, and ? It already gets information about dropped columns, if that is the third one. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Idea: closing the loop for "pg_ctl reload"
On 3/3/15 11:15 AM, Jan de Visser wrote: On March 3, 2015 11:09:29 AM Jim Nasby wrote: On 3/3/15 9:26 AM, Andres Freund wrote: On 2015-03-03 15:21:24 +, Greg Stark wrote: Fwiw this concerns me slightly. I'm sure a lot of people are doing things like "kill -HUP `cat .../postmaster.pid`" or the equivalent. postmaster.pid already contains considerably more than just the pid. e.g. 4071 /srv/dev/pgdev-master 1425396089 5440 /tmp localhost 5440001 82345992 If we already have all this extra stuff, why not include an actual error message then, or at least the first line of an error (or maybe just swap any newlines with spaces)? Not impossible. I can play around with that and see if it's as straightforward as I think it is. I'm sure the code side of this is trivial; it's a question of why Tom was objecting. It would probably be better for us to come to a conclusion before working on this. On a related note... something else we could do here would be to keep a last-known-good copy of the config files around. That way if you flubbed something at least the server would still start. I do think that any admin worth anything would notice an error from pg_ctl, but maybe others have a different opinion. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] logical column ordering
On 3/3/15 11:23 AM, Bruce Momjian wrote: On Thu, Feb 26, 2015 at 01:55:44PM -0800, Josh Berkus wrote: On 02/26/2015 01:54 PM, Alvaro Herrera wrote: This patch decouples these three things so that they can changed freely -- but provides no user interface to do so. I think that trying to only decouple the thing we currently have in two pieces, and then have a subsequent patch to decouple again, is additional conceptual complexity for no gain. Oh, I didn't realize there weren't commands to change the LCO. Without at least SQL syntax for LCO, I don't see why we'd take it; this sounds more like a WIP patch. FYI, pg_upgrade is going to need pg_dump --binary-upgrade to output the columns in physical order with some logical ordering information, i.e. pg_upgrade cannot be passed only logical ordering from pg_dump. Wouldn't it need attno info too, so all 3 orderings? -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] logical column ordering
On Thu, Feb 26, 2015 at 01:55:44PM -0800, Josh Berkus wrote: > On 02/26/2015 01:54 PM, Alvaro Herrera wrote: > > This patch decouples these three things so that they > > can changed freely -- but provides no user interface to do so. I think > > that trying to only decouple the thing we currently have in two pieces, > > and then have a subsequent patch to decouple again, is additional > > conceptual complexity for no gain. > > Oh, I didn't realize there weren't commands to change the LCO. Without > at least SQL syntax for LCO, I don't see why we'd take it; this sounds > more like a WIP patch. FYI, pg_upgrade is going to need pg_dump --binary-upgrade to output the columns in physical order with some logical ordering information, i.e. pg_upgrade cannot be passed only logical ordering from pg_dump. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Idea: closing the loop for "pg_ctl reload"
On March 3, 2015 11:09:29 AM Jim Nasby wrote: > On 3/3/15 9:26 AM, Andres Freund wrote: > > On 2015-03-03 15:21:24 +, Greg Stark wrote: > >> Fwiw this concerns me slightly. I'm sure a lot of people are doing > >> things like "kill -HUP `cat .../postmaster.pid`" or the equivalent. > > > > postmaster.pid already contains considerably more than just the pid. e.g. > > 4071 > > /srv/dev/pgdev-master > > 1425396089 > > 5440 > > /tmp > > localhost > > > >5440001 82345992 > > If we already have all this extra stuff, why not include an actual error > message then, or at least the first line of an error (or maybe just swap > any newlines with spaces)? Not impossible. I can play around with that and see if it's as straightforward as I think it is. -- 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] Idea: closing the loop for "pg_ctl reload"
On March 3, 2015 10:29:43 AM Tom Lane wrote: > Andres Freund writes: > > On 2015-03-03 15:21:24 +, Greg Stark wrote: > >> Fwiw this concerns me slightly. I'm sure a lot of people are doing > >> things like "kill -HUP `cat .../postmaster.pid`" or the equivalent. > > > > postmaster.pid already contains considerably more than just the pid. e.g. > > Yeah, that ship sailed long ago. I'm sure anyone who's doing this is > using "head -1" not just "cat", and what I suggest wouldn't break that. > > regards, tom lane And what I've implemented doesn't either. The extra line is added to the back of the file. -- 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] Idea: closing the loop for "pg_ctl reload"
On 3/3/15 9:26 AM, Andres Freund wrote: On 2015-03-03 15:21:24 +, Greg Stark wrote: Fwiw this concerns me slightly. I'm sure a lot of people are doing things like "kill -HUP `cat .../postmaster.pid`" or the equivalent. postmaster.pid already contains considerably more than just the pid. e.g. 4071 /srv/dev/pgdev-master 1425396089 5440 /tmp localhost 5440001 82345992 If we already have all this extra stuff, why not include an actual error message then, or at least the first line of an error (or maybe just swap any newlines with spaces)? -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] autogenerated column names + views are a dump hazard
On 2015-03-03 11:09:29 -0500, Tom Lane wrote: > Andres Freund writes: > > On 2015-03-03 09:39:03 -0500, Tom Lane wrote: > > I think this is the way to go though. There's different extremes we can > > go to though - the easiest is to simply remove the attname = "?column?" > > assignment from get_target_list(). That way plain Var references (aside > > from whole row vars/subplans and such that get_variable() deals with) > > don't get a forced alias, but everything else does. That seems like a > > good compromise between readability and safety. get_rule_expr() deals > > with most of the "nasty" stuff. > > I wasn't aware that there was any room for "compromise" on the safety > aspect. Meh. I think that *not* printing an alias for a plain column reference is reasonable and doesn't present a relevant risk even in the face of future changes. "foo.bar AS bar" is just a bit too redundant imo. > That's why I was thinking that relying on the pretty flag might be a > reasonable thing. pg_dump would be unconditionally right, and we'd > not uglify EXPLAIN or \d+ output. I don't think EXPLAIN VERBOSE currently is affected - it doesn't seem to show aliases/generated column names at all. I personally do occasionally copy & paste from from \d+ output, so I'd rather have slightly more verbose output rather than wrong output. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Maximum number of WAL files in the pg_xlog directory
On Tue, Oct 14, 2014 at 01:21:53PM -0400, Bruce Momjian wrote: > On Tue, Oct 14, 2014 at 09:20:22AM -0700, Jeff Janes wrote: > > On Mon, Oct 13, 2014 at 12:11 PM, Bruce Momjian wrote: > > > > > > I looked into this, and came up with more questions. Why is > > checkpoint_completion_target involved in the total number of WAL > > segments? If checkpoint_completion_target is 0.5 (the default), the > > calculation is: > > > > (2 + 0.5) * checkpoint_segments + 1 > > > > while if it is 0.9, it is: > > > > (2 + 0.9) * checkpoint_segments + 1 > > > > Is this trying to estimate how many WAL files are going to be created > > during the checkpoint? If so, wouldn't it be (1 + > > checkpoint_completion_target), not "2 +". My logic is you have the old > > WAL files being checkpointed (that's the "1"), plus you have new WAL > > files being created during the checkpoint, which would be > > checkpoint_completion_target * checkpoint_segments, plus one for the > > current WAL file. > > > > > > WAL is not eligible to be recycled until there have been 2 successful > > checkpoints. > > > > So at the end of a checkpoint, you have 1 cycle of WAL which has just become > > eligible for recycling, > > 1 cycle of WAL which is now expendable but which is kept anyway, and > > checkpoint_completion_target worth of WAL which has occurred while the > > checkpoint was occurring and is still needed for crash recovery. > > OK, so based on this analysis, what is the right calculation? This? > > (1 + checkpoint_completion_target) * checkpoint_segments + 1 + > max(wal_keep_segments, checkpoint_segments) Now that we have min_wal_size and max_wal_size in 9.5, I don't see any value to figuring out the proper formula for backpatching. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New CF app deployment
On Tue, Mar 3, 2015 at 10:58:28AM -0500, Bruce Momjian wrote: > > Would you suggest removing the automated system completely, or keep it > > around > > and just make it possible to override it (either by removing the note that > > something is a patch, or by making something that's not listed as a patch > > become marked as such)? > > One counter-idea would be to assume every attachment is a patch _unless_ > the attachment type matches a pattern that identifies it as not a patch. > > However, I agree with Tom that we should go a little longer before > changing it. Also, can we look inside the attachment to see if it starts with 'diff'. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New CF app deployment
On Sun, Feb 22, 2015 at 03:12:12PM -0500, Magnus Hagander wrote:
> > # Attempt to identify the file using magic information
> > mtype = mag.buffer(contents)
> > if mtype.startswith('text/x-diff'):
> > a.ispatch = True
> > else:
> > a.ispatch = False
...
>
> I think the old system where the patch submitter declared, this message
> contains my patch, is the only one that will work.
>
>
>
> Would you suggest removing the automated system completely, or keep it around
> and just make it possible to override it (either by removing the note that
> something is a patch, or by making something that's not listed as a patch
> become marked as such)?
One counter-idea would be to assume every attachment is a patch _unless_
the attachment type matches a pattern that identifies it as not a patch.
However, I agree with Tom that we should go a little longer before
changing it.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] autogenerated column names + views are a dump hazard
Andres Freund writes: > On 2015-03-03 09:39:03 -0500, Tom Lane wrote: >> And on the third hand ... doing that would really only be robust as long >> as you assume that the output will be read by a server using exactly the >> same FigureColname() logic as what we are using. So maybe the whole idea >> is a bad one, and we should just bite the bullet and print AS clauses >> always. > I think this is the way to go though. There's different extremes we can > go to though - the easiest is to simply remove the attname = "?column?" > assignment from get_target_list(). That way plain Var references (aside > from whole row vars/subplans and such that get_variable() deals with) > don't get a forced alias, but everything else does. That seems like a > good compromise between readability and safety. get_rule_expr() deals > with most of the "nasty" stuff. I wasn't aware that there was any room for "compromise" on the safety aspect. If pg_dump gets this wrong, that means pg_upgrade is broken, for example. That's why I was thinking that relying on the pretty flag might be a reasonable thing. pg_dump would be unconditionally right, and we'd not uglify EXPLAIN or \d+ output. 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] autogenerated column names + views are a dump hazard
On 2015-03-03 09:39:03 -0500, Tom Lane wrote: > Andres Freund writes: > > For this case it seems easiest if we'd make get_rule_expr() (and some of > > its helpers) return whether an implicit cast has been added. > > Aside from being pretty ugly, that doesn't seem particularly > bulletproof. Right. > It might deal all right with this specific manifestation, but now that > we've seen the problem, who's to say that there aren't other ways to get > bit? Or that some innocent future change to FigureColname() might not > create a new one? It's not like the behavior of that function was handed > down on stone tablets --- we do change it from time to time. Sure it'd not be a guarantee, but what is? > I wondered whether there was a way to directly test whether > FigureColname() gives a different result from what we have recorded > as the effective column name. Unfortunately, it wants a raw parse tree > whereas what we have is an analyzed parse tree, so there's no easy way > to apply it and see. Additionally the casts added by get_rule_expr() show up in neither tree, so that'd not in itself help. > Now, we do have the deparsed text of the column expression at hand, > so in principle you could run that back through the grammar to get a raw > parse tree, and then apply FigureColname() to that. Not sure what that > would be like from a performance standpoint, but it would provide a pretty > robust fix for this class of problem. Yea, I've wondered about that as well. Given that this is pretty much only run during deparsing I don't think the overhead would be relevant. > And on the third hand ... doing that would really only be robust as long > as you assume that the output will be read by a server using exactly the > same FigureColname() logic as what we are using. So maybe the whole idea > is a bad one, and we should just bite the bullet and print AS clauses > always. I think this is the way to go though. There's different extremes we can go to though - the easiest is to simply remove the attname = "?column?" assignment from get_target_list(). That way plain Var references (aside from whole row vars/subplans and such that get_variable() deals with) don't get a forced alias, but everything else does. That seems like a good compromise between readability and safety. get_rule_expr() deals with most of the "nasty" stuff. I did this, and the noise in the regression test output isn't too bad. Primarily that a fair number of of EXISTS(SELECT 1 .. ) grow an alias. > Or we could consider printing them always if the "pretty" flag isn't > on. IIRC, pg_dump doesn't enable that. Not a fan of that, I'd rather not output queries that can't be executed. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
