Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

2017-01-23 Thread Corey Huinker
On Tue, Jan 24, 2017 at 1:15 AM, Fabien COELHO  wrote:

>
> 1: unrecognized value "whatever" for "\if"; assuming "on"
>>>
>>> I do not think that the script should continue with such an assumption.
>>>
>>
>> I agree, and this means we can't use ParseVariableBool() as-is. I already
>> broke out argument reading to it's own function, knowing that it'd be the
>> stub for expressions. So I guess we start that now. What subset of true-ish
>> values do you think we should support? If we think that real expressions
>> are possible soon, we could only allow 'true' and 'false' for now, but if
>> we expect that expressions might not make it into v10, then perhaps we
>> should support the same text values that coerce to booleans on the server
>> side.
>>
>
> Hmmm. I would text value that coerce to true? I would also accept non-zero
> integers (eg SELECT 1::BOOL; -- t).
>


The docs (doc/src/sgml/datatype.sgml) list TRUE 't' 'true' 'y' 'yes' 'on'
'1'vs FALSE 'f' 'false' 'n' 'no' 'off' '0'

However, src/test/regress/expected/boolean.out shows that there's some
flexiblity there with spaces, even before you inspect parse_bool_with_len()
in src/backend/utils/adt/bool.c.

If we could bring src/backend/utils/adt/bool.c across the server/client
wall, I would just use parse_bool_with_len as-is.

As it is, given that whatever I do is temporary until real expressions come
into place, that wouldn't be a terrible amount of code copying, and we'd
still have a proto-expression that probably isn't going to conflict with
whatever expression syntax we do chose. Having said that, if anyone can see
ANY reason that some subset of the existing bool-friendly strings would
cause a problem with the expression syntax we do hope to use, speak now and
we can restrict the accepted values.


> I would suggest to assume false on everything else, and/or maybe to ignore
> the whole if/endif section in such cases.


+1, it also halves the number of values we have to support later.


> ISTM that with TAP test you can check for error returns, so maybe this can
> be done.


I'll have to educate myself on TAP tests.


Re: [HACKERS] pg_hba_file_settings view patch

2017-01-23 Thread Michael Paquier
On Mon, Jan 23, 2017 at 5:13 PM, Haribabu Kommi
 wrote:
> On Sat, Jan 21, 2017 at 8:01 AM, Tom Lane  wrote:
>> * I'm not exactly convinced that the way you approached the error message
>> reporting, ie duplicating the logged message, is good.  In particular
>> this results in localizing the strings reported in pg_hba_rules.error,
>> which is exactly opposite to the decision we reached for the
>> pg_file_settings view.  What's the reasoning for deciding that this
>> view should contain localized strings?  (More generally, we found in
>> the pg_file_settings view that we didn't always want to use exactly
>> the same string that was logged, anyway.)
>
> Actually there is no particular reason to display the localized strings,
> Just thought that it may be useful to the user if it get displayed in their
> own language. And also doing this way will reduce the error message
> duplicate in the code that is used for display in the view and writing it
> into the log file.

Perhaps consistency would not hurt and something like
record_config_file_error() could be done to save the error parsing
error. What's actually the problem with localized strings exposed in a
system view? Encoding conflicts?

>> * Also, there seems to be a lot of ereports remaining unconverted,
>> eg the "authentication file token too long" error.  One of the things
>> we wanted pg_file_settings to be able to do was finger pretty much any
>> mistake in the config file, including syntax errors.  It seems like
>> it'd be a shame if pg_hba_rules is unable to help with that.  You
>> should be able to fill in line number and error even if the line is
>> too mangled to be able to populate the other fields sanely.
>
> The two errors that are missed are, "could not open secondary authentication
> file"
> and "authentication file token too long" errors. For these two cases, the
> server
> is not throwing any error, it just logs the message and continues. Is it
> fine to add
> these these two cases as errors in the view?

Missed those ones during the initial review... It would be a good idea
to include them to track problems.
-- 
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] WIP: About CMake v2

2017-01-23 Thread Michael Paquier
On Tue, Jan 24, 2017 at 3:58 PM, Andres Freund  wrote:
> On 2017-01-24 15:50:47 +0900, Michael Paquier wrote:
>> I am marking this patch as "returned with feedback". That's quite a
>> heavy change and it looks to be too late in the development cycle of
>> PG10 to consider it. Peter's commit bits, who is also the reviewer,
>> are beginning to smoke as well after everything that has happened for
>> the logical replication changes.
>
> I'm doubtful about this being ready in time too, but it seems a might
> heavyhanded to make that call on your own. Including the judgement about
> Peter's capability to handle more.

Sure, sorry for that. I am switching back the patch, let's see what
happens by the end of the CF.
-- 
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] WIP: About CMake v2

2017-01-23 Thread Andres Freund
On 2017-01-24 15:50:47 +0900, Michael Paquier wrote:
> I am marking this patch as "returned with feedback". That's quite a
> heavy change and it looks to be too late in the development cycle of
> PG10 to consider it. Peter's commit bits, who is also the reviewer,
> are beginning to smoke as well after everything that has happened for
> the logical replication changes.

I'm doubtful about this being ready in time too, but it seems a might
heavyhanded to make that call on your own. Including the judgement about
Peter's capability to handle more.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Odd plan shown in src/backend/optimizer/README

2017-01-23 Thread Etsuro Fujita

On 2017/01/23 23:39, Tom Lane wrote:

Etsuro Fujita  writes:

This seems odd to me; we would not need the bottom-level Nestloop.
Attached is a small patch for fixing that.



Pushed, thanks.


Thank you for picking this up!

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] Checksums by default?

2017-01-23 Thread Andres Freund
On 2017-01-23 21:11:37 -0600, Merlin Moncure wrote:
> End user data damage ought to prevented at all costs IMO.

Really, really, really not. We should do a lot, but if that'd be the
only priority we'd enable all the expensive as shit stuff and be so slow
that there'd be no users.  We'd never add new scalability/performance
features, because they'll initially have more bugs / increase
complexity. Etc.

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] WIP: About CMake v2

2017-01-23 Thread Michael Paquier
On Tue, Jan 3, 2017 at 11:11 PM, Peter Eisentraut
 wrote:
> On 12/30/16 9:10 AM, Yuriy Zhuravlev wrote:
>> cmake_v2_2_c_define.patch
>>
>> Small chages in c.h . At first it is “#pragma fenv_access (off)” it is
>> necessary if we use /fp:strict for MSVC compiler. Without this pragma we
>> can’t calc floats for const variables in compiller time (2 * M_PI for
>> example). Strict mode important if we want to be close with ieee754
>> float format on MSVC (1.0 / 0.0 = inf for example).  Detail info here:
>> https://msdn.microsoft.com/en-us/library/e7s85ffb.aspx
>
> I don't understand what this has to do with cmake.  If this is a
> worthwhile improvement for the Windows build, then please explain why,
> with a "before" and "after" output and a patch for the existing build
> system as well.
>
>> Second change is because I find and set HAVE_INT128 directly from CMake.
>> PG_INT128_TYPE used only for autoconfig scripts.
>
> It might also be worth refactoring the existing Autoconf code here to
> make this consistent.
>
> (My assumption is that if we were to move forward with cmake or any
> other build system change, we would have to keep the old one alongside
> at least for a little while.  So any changes to the C code would need to
> be side-ported.)
>
>> cmake_v2_3_rijndael.patch
>>
>> First I added special wraparound because here CMake have circular
>> dependency (cmake very smart here). Second I removed rijndael.tbl
>> because it generated during build process every time.
>
> Please explain what the circular dependency is.  If there is one, we
> should also side-port this change.
>
>> cmake_v2_4_uuid.patch
>>
>> Another small patch. Right place for uuid.h I find by CMake and not
>> necessary this ifdef hell.
>
> This patch removes the uuid.h include but doesn't add it anywhere else.
> How does it work?
>
>> Questions for discussion:
>>
>> In generated project by CMake we always have only one enter point. Also
>> INSTALL macross support only including to “all” targets. It follows that
>> it is impossible build contrib modules separately only with “all”
>> target. Here write about this behavior:
>> https://cmake.org/cmake/help/v3.7/prop_tgt/EXCLUDE_FROM_ALL.html
>
> Yeah, I think this is how the MSVC stuff effectively works right now as
> well.

I am marking this patch as "returned with feedback". That's quite a
heavy change and it looks to be too late in the development cycle of
PG10 to consider it. Peter's commit bits, who is also the reviewer,
are beginning to smoke as well after everything that has happened for
the logical replication changes.
-- 
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] Speedup twophase transactions

2017-01-23 Thread Michael Paquier
On Mon, Jan 23, 2017 at 9:00 PM, Nikhil Sontakke
 wrote:
> Hi Stas,
>
>>
>> So, here is brand new implementation of the same thing.
>>
>> Now instead of creating pgproc entry for prepared transaction during 
>> recovery,
>> I just store recptr/xid correspondence in separate 2L-list and
>> deleting entries in that
>> list if redo process faced commit/abort. In case of checkpoint or end
>> of recovery
>> transactions remaining in that list are dumped to files in pg_twophase.
>>
>> Seems that current approach is way more simpler and patch has two times less
>> LOCs then previous one.
>>
>
> The proposed approach and patch does appear to be much simpler than
> the previous one.
>
> I have some comments/questions on the patch (twophase_recovery_list_2.diff):
>
> 1)
> +   /*
> +* Move prepared transactions from KnownPreparedList to files, if any.
> +* It is possible to skip that step and teach subsequent code about
> +* KnownPreparedList, but whole PrescanPreparedTransactions() happens
> +* once during end of recovery or promote, so probably it isn't worth
> +* complications.
> +*/
> +   KnownPreparedRecreateFiles(InvalidXLogRecPtr);
> +
>
> Speeding up recovery or failover activity via a faster promote is a
> desirable thing. So, maybe, we should look at teaching the relevant
> code about using "KnownPreparedList"? I know that would increase the
> size of this patch and would mean more testing, but this seems to be
> last remaining optimization in this code path.

That's a good idea, worth having in this patch. Actually we may not
want to call KnownPreparedRecreateFiles() here as promotion is not
synonym of end-of-recovery checkpoint for a couple of releases now.

> 3) We are pushing the fsyncing of 2PC files to the checkpoint replay
> activity with this patch. Now, typically, we would assume that PREPARE
> followed by COMMIT/ABORT would happen within a checkpoint replay
> cycle, if not, this would make checkpoint replays slower since the
> earlier spread out fsyncs are now getting bunched up. I had concerns
> about this but clearly your latest performance measurements don't show
> any negatives around this directly.

Most of the 2PC syncs just won't happen, such transactions normally
don't last long, and the number you would get during a checkpoint is
largely lower than what would happen between two checkpoints. When
working on Postgres-XC, the number of 2PC was really more funky.

> 6) Do we need to hold TwoPhaseStateLock lock in shared mode while
> calling KnownPreparedRecreateFiles()? I think, it does not matter in
> the recovery/replay code path.

It does not matter as the only code path tracking that would be the
checkpointer in TwoPhaseCheckpoint(), and this 2PC recovery patch does
not touch anymore TwoPhaseStateData. Actually the patch makes no use
of it.

> 7) I fixed some typos and comments. PFA, patch attached.
>
> Other than this, I ran TAP tests and they succeed as needed.
>
> On 23 January 2017 at 16:56, Stas Kelvich  wrote:
>> I did tests with following setup:
>>
>> * Start postgres initialised with pgbench
>> * Start pg_receivexlog
>> * Take basebackup
>> * Perform 1.5 M transactions
>> * Stop everything and apply wal files stored by pg_receivexlog to base 
>> backup.
>>
>> All tests performed on a laptop with nvme ssd
>> number of transactions: 1.5M
>> start segment: 0x4
>>
>> -master non-2pc:
>> last segment: 0x1b
>> average recovery time per 16 wal files: 11.8s
>> average total recovery time: 17.0s
>>
>> -master 2pc:
>> last segment: 0x44
>> average recovery time per 16 wal files: 142s
>> average total recovery time: 568s
>>
>> -patched 2pc (previous patch):
>> last segment: 0x44
>> average recovery time per 16 wal files: 5.3s
>> average total recovery time: 21.2s
>>
>> -patched2 2pc (dlist_push_tail changed to dlist_push_head):
>> last segment: 0x44
>> average recovery time per 16 wal files: 5.2s
>> average total recovery time: 20.8s

The difference between those two is likely noise.

By the way, in those measurements, the OS cache is still filled with
the past WAL segments, which is a rather best case, no? What happens
if you do the same kind of tests on a box where memory is busy doing
something else and replayed WAL segments get evicted from the OS cache
more aggressively once the startup process switches to a new segment?
This could be tested for example on a VM with few memory (say 386MB or
less) so as the startup process needs to access again the past WAL
segments to recover the 2PC information it needs to get them back
directly from disk... One trick that you could use here would be to
tweak the startup process so as it drops the OS cache once a segment
is finished replaying, and see the effects of an aggressive OS cache
eviction. This patch is showing really nice improvements with the OS
cache backing up the data, still it would 

Re: [HACKERS] contrib modules and relkind check

2017-01-23 Thread Amit Langote
On 2017/01/24 15:11, Michael Paquier wrote:
> On Tue, Jan 24, 2017 at 2:14 PM, Amit Langote
>  wrote:
>> Some contrib functions fail to fail sooner when relations of unsupported
>> relkinds are passed, resulting in error message like one below:
>>
>> create table foo (a int);
>> create view foov as select * from foo;
>> select pg_visibility('foov', 0);
>> ERROR:  could not open file "base/13123/16488": No such file or directory
>>
>> Attached patch fixes that for all such functions I could find in contrib.
>>
>> It also installs RELKIND_PARTITIONED_TABLE as unsupported in a couple of
>> places (in pageinspect and pgstattuple).
> 
> I have spent some time looking at your patch, and did not find any
> issues with it, nor did I notice code paths that were not treated or
> any other contrib modules sufferring from the same deficiencies that
> you may have missed. Nice work.

Thanks for the review, Michael!

Regards,
Amit




-- 
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] IndexBuild Function call fcinfo cannot access memory

2017-01-23 Thread Jia Yu
Dear Hackers,

Sorry for bothering you again!

I have migrated my index method from PG 9.5 to PG 9.6.1 as Tom suggested.
The same problem still exists. The index works fine if I just call it
through my new SQL script in regression test (call "make check").

I am doubting whether this is caused by some security issues.

I compiled the code on PG 9.6.1 with flag -O0. It gave me this same error.
The new backtraces are as this:

---

#0  0x0046bfbd in hippobuild (heap=, index=, indexInfo=) at hippo.c:204
#1  0x0055d1c9 in index_build (heapRelation=0x7f0f3bdfea88,
indexRelation=0x7f0f3be002b0, indexInfo=0x2e1d180, isprimary=0 '\000',
isreindex=0 '\000') at index.c:2021
#2  0x0055beda in index_create (heapRelation=0x7f0f3bdfea88,
indexRelationName=0x2ec2378 "hippo_idx", indexRelationId=20695,
relFileNode=0, indexInfo=0x2e1d180, indexColNames=0x2ec0370,
accessMethodObjectId=9000, tableSpaceId=0, collationObjectId=0x2ec0938,
classObjectId=0x2ec0958,
coloptions=0x2ec0978, reloptions=49030800, isprimary=0 '\000',
isconstraint=0 '\000', deferrable=0 '\000', initdeferred=0 '\000',
allow_system_table_mods=0 '\000', skip_build=0 '\000', concurrent=0 '\000',
is_internal=0 '\000', if_not_exists=0 '\000') at index.c:1099
#3  0x00632d7a in DefineIndex (relationId=12503, stmt=0x2e1d298,
indexRelationId=0, is_alter_table=0 '\000', check_rights=1 '\001',
skip_build=0 '\000', quiet=0 '\000') at indexcmds.c:651
#4  0x0083660e in ProcessUtilitySlow (parsetree=0x2e7ba00,
queryString=0x2e7acc8 "create index hippo_idx on hippo_tbl using hippo(id2)
with (density=20);\n", context=PROCESS_UTILITY_TOPLEVEL, params=0x0,
dest=0xe68c60 , completionTag=0x7ffc613bf280 "") at
utility.c:1274
#5  0x00835a98 in standard_ProcessUtility (parsetree=0x2e7ba00,
queryString=0x2e7acc8 "create index hippo_idx on hippo_tbl using hippo(id2)
with (density=20);\n", context=PROCESS_UTILITY_TOPLEVEL, params=0x0,
dest=0xe68c60 , completionTag=0x7ffc613bf280 "") at
utility.c:907
#6  0x00834b71 in ProcessUtility (parsetree=0x2e7ba00,
queryString=0x2e7acc8 "create index hippo_idx on hippo_tbl using hippo(id2)
with (density=20);\n", context=PROCESS_UTILITY_TOPLEVEL, params=0x0,
dest=0xe68c60 , completionTag=0x7ffc613bf280 "") at
utility.c:336
#7  0x00833c1b in PortalRunUtility (portal=0x2eba198,
utilityStmt=0x2e7ba00, isTopLevel=1 '\001', setHoldSnapshot=0 '\000',
dest=0xe68c60 , completionTag=0x7ffc613bf280 "") at
pquery.c:1193
#8  0x00833e2e in PortalRunMulti (portal=0x2eba198, isTopLevel=1
'\001', setHoldSnapshot=0 '\000', dest=0xe68c60 ,
altdest=0xe68c60 , completionTag=0x7ffc613bf280 "") at
pquery.c:1342
#9  0x0083332f in PortalRun (portal=0x2eba198,
count=9223372036854775807, isTopLevel=1 '\001', dest=0xe68c60 ,
altdest=0xe68c60 , completionTag=0x7ffc613bf280 "") at
pquery.c:815
#10 0x0082d05b in exec_simple_query (query_string=0x2e7acc8 "create
index hippo_idx on hippo_tbl using hippo(id2) with (density=20);\n") at
postgres.c:1094
#11 0x00831479 in PostgresMain (argc=6, argv=0x2df5a70,
dbname=0x2dfd3c0 "postgres", username=0x2dfd3c0 "postgres") at
postgres.c:4070
#12 0x006e7832 in main (argc=6, argv=0x2df5a70) at main.c:224

---

The problematic line is at Hippo.c Line 204. Here is the code of that line.
It is the regular routine required by designing an index access method. It
will build the index.

--
/*
 *This function initializes the entire Hippo. It will call buildcallback
many times.
 */
IndexBuildResult *
hippobuild(Relation heap, Relation index, IndexInfo *indexInfo)
*Line 204:*{
IndexBuildResult *result;
double reltuples;
HippoBuildState buildstate;
Buffer buffer;
Datum *histogramBounds;
int histogramBoundsNum,i;
HeapTuple heapTuple;
Page page;/* Initial page */
Size pageSize;
AttrNumber attrNum;

--

This is my SQL script which works fine when running in PG regression test
parallel_schedule. But it fails at normal PG server.

--

create table hippo_tbl(id int8, id2 int8, payload text);
insert into hippo_tbl(id, id2, payload) select i, random()*100,
repeat('a', 100) from generate_series (1,100) i;
Alter table hippo_tbl alter column id2 set statistics 400;
Analyze hippo_tbl;
create index hippo_idx on hippo_tbl using hippo(id2) with (density=20);
select count(*) from hippo_tbl where id2>10 and id2 <101000;
insert into hippo_tbl(id, id2, payload) select i, 19, repeat('a', 100)
from generate_series (1, 1000) i;
select count(*) from hippo_tbl where id2>10 and id2 <101000;
delete from hippo_tbl where  id2>10 and id2 <101000;
VACUUM;
select count(*) from hippo_tbl where id2>10 and id2 <101000;
drop index hippo_idx;
drop table hippo_tbl;
--

Any help will be appreciated!

Thanks,
Jia Yu


On Mon, Jan 23, 2017 at 5:12 PM, Jia Yu 

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

2017-01-23 Thread Corey Huinker
>
>
> As I understood it, the negative feeback was really about sh inspiration
> "if/fi", not about elif/elsif/elseif. I do not think that there was an
> expressed consensus about the later.
>

True


> The closest case is CPP with its line-oriented #-prefix syntax, and I
> still think that we should do it like that.


Given that the pl/pgsql syntax has a space between "end" and "if", I have
to agree. It's easy enough to change back if others can make a convincing
argument for something else.


Re: [HACKERS] pageinspect: Hash index support

2017-01-23 Thread Amit Kapila
On Tue, Jan 24, 2017 at 11:41 AM, Ashutosh Sharma  wrote:
>>> Secondly, we will have to input overflow block number as an input to
>>> this function so as to determine the overflow bit number which can be
>>> used further to identify the bitmap page.
>>>
>>
>> I think you can get that from bucket number by using BUCKET_TO_BLKNO.
>> You can get bucket number from page's opaque data.  So, if we follow
>> that then you can have a prototype of a function as
>> hash_bitmap_info(index_oid, page bytea) which will be quite similar to
>> other API's exposed by this module.
>>
>
> AFAIU, BUCKET_TO_BLKNO will give us the block number of a primary
> bucket page rather than overflow page and what we want is an overflow
> block number so as to determine the overflow bit number which can be
> used further to identify the bitmap page.
>

Right.

> If we pass overflow page as
> an input to hash_bitmap_info() we won't be able to get it's block
> number.
>

I think we can get it by using its prev block number, but not sure if
it is worth the complexity, so let's keep the interface as proposed by
you.  TBH, I am not sure how important is to expose this
(hash_bitmap_info()) function, but let's keep it and see if committer
has any opinion on this API.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

2017-01-23 Thread Fabien COELHO



1: unrecognized value "whatever" for "\if"; assuming "on"

I do not think that the script should continue with such an assumption.


I agree, and this means we can't use ParseVariableBool() as-is. I 
already broke out argument reading to it's own function, knowing that 
it'd be the stub for expressions. So I guess we start that now. What 
subset of true-ish values do you think we should support? If we think 
that real expressions are possible soon, we could only allow 'true' and 
'false' for now, but if we expect that expressions might not make it 
into v10, then perhaps we should support the same text values that 
coerce to booleans on the server side.


Hmmm. I would text value that coerce to true? I would also accept non-zero 
integers (eg SELECT 1::BOOL; -- t).


I would suggest to assume false on everything else, and/or maybe to ignore 
the whole if/endif section in such cases.



All valid issues. Will add those to the regression as well (with
ON_ERROR_STOP disabled, obviously).


ISTM that with TAP test you can check for error returns, so maybe this can 
be done.


--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] contrib modules and relkind check

2017-01-23 Thread Michael Paquier
On Tue, Jan 24, 2017 at 2:14 PM, Amit Langote
 wrote:
> Some contrib functions fail to fail sooner when relations of unsupported
> relkinds are passed, resulting in error message like one below:
>
> create table foo (a int);
> create view foov as select * from foo;
> select pg_visibility('foov', 0);
> ERROR:  could not open file "base/13123/16488": No such file or directory
>
> Attached patch fixes that for all such functions I could find in contrib.
>
> It also installs RELKIND_PARTITIONED_TABLE as unsupported in a couple of
> places (in pageinspect and pgstattuple).

I have spent some time looking at your patch, and did not find any
issues with it, nor did I notice code paths that were not treated or
any other contrib modules sufferring from the same deficiencies that
you may have missed. Nice work.
-- 
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] pageinspect: Hash index support

2017-01-23 Thread Ashutosh Sharma
>> Secondly, we will have to input overflow block number as an input to
>> this function so as to determine the overflow bit number which can be
>> used further to identify the bitmap page.
>>
>
> I think you can get that from bucket number by using BUCKET_TO_BLKNO.
> You can get bucket number from page's opaque data.  So, if we follow
> that then you can have a prototype of a function as
> hash_bitmap_info(index_oid, page bytea) which will be quite similar to
> other API's exposed by this module.
>

AFAIU, BUCKET_TO_BLKNO will give us the block number of a primary
bucket page rather than overflow page and what we want is an overflow
block number so as to determine the overflow bit number which can be
used further to identify the bitmap page. If we pass overflow page as
an input to hash_bitmap_info() we won't be able to get it's block
number. Considering above facts, I think we need to input overflow
block number rather than overflow page to hash_bitmap_info(). Please
let me know your opinion on this. Thanks.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

2017-01-23 Thread Fabien COELHO


Hello,


I'm personally indifferent to elif vs elsif vs elseif, but I think elseif
was the consensus. It's easy enough to change.


Went with elsif to follow pl/pgsql. In reviewing the original thread it
seemed that "elif" was linked to "fi" and that got some negative feedback.


As I understood it, the negative feeback was really about sh inspiration 
"if/fi", not about elif/elsif/elseif. I do not think that there was an

expressed consensus about the later.

Else-if variants from different languages include:

 VB: If ElseIf Else End If (with optional Then)
 PHP: if {} elseif {} else {}
 Tcl: if {} elseif {} else {}

 PL/pgSQL: IF ... THEN ... ELSIF ... ELSE ... END IF;
 Perl: if {} elsif {} else {}
 Ruby: if elsif else end

 CPP: #if #elif #else #endif
 Python: if : elif: else:
 bash: if [] then ... elif ... else ... fi

The closest case is CPP with its line-oriented #-prefix syntax, and I 
still think that we should do it like that.



I went looking for other examples of explicit /* PASSTHROUGH */ comments
and could find none. Could you explain what it is you want me to fix with
the switch statements?


Sorry, I got it wrong. The comment (which is FALLTHROUGH or FALL THROUGH, 
not PASSTHROUGH) is required if there is specific code in a case and the 
case is expected to continue with executing the next case code as well. 
This is quite rare, and it is not the case for your code, which just has 
some comments in one case.


--
Fabien


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

2017-01-23 Thread Pavel Stehule
2017-01-23 21:59 GMT+01:00 Jim Nasby :

> On 1/23/17 2:10 PM, Pavel Stehule wrote:
>
>> Comments, notes?
>>
>
> +1 on the idea. It'd also be nice if we could expose control of plans for
> dynamic SQL, though I suspect that's not terribly useful without some kind
> of global session storage.
>

yes - it requires classic normalised query string controlled plan cache.

But same plan cache options can be valid for prepared statements - probably
with GUC. This is valid theme, but out of my proposal in this moment.

Regards

Pavel


Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

2017-01-23 Thread Pavel Stehule
Hi

2017-01-23 21:59 GMT+01:00 Jim Nasby :

> On 1/23/17 2:10 PM, Pavel Stehule wrote:
>
>> Comments, notes?
>>
>
> +1 on the idea. It'd also be nice if we could expose control of plans for
> dynamic SQL, though I suspect that's not terribly useful without some kind
> of global session storage.
>
> A couple notes on a quick read-through:
>
> Instead of paralleling all the existing namespace stuff, I wonder if it'd
> be better to create explicit block infrastructure. AFAIK PRAGMAs are going
> to have a lot of the same requirements (certainly the nesting is the same),
> and we might want more of this king of stuff in the future. (I've certainly
> wished I could set a GUC in a plpgsql block and have it's settings revert
> when exiting the block...)
>

I am not sure if I understand. ?? Setting GUC by PRAGMA can work - the
syntax supports it and GUC API supports nesting. Not sure about exception
handling - but it should not be problem probably.

Please, can you show some examples.


> Perhaps that's as simple as renaming all the existing _ns_* functions to
> _block_ and then adding support for pragmas...
>
> Since you're adding cursor_options to PLpgSQL_expr it should probably be
> removed as an option to exec_*.
>

I have to recheck it. Some cursor options going from dynamic cursor
variables and are related to dynamic query - not query that creates query
string.

>
> finit_ would be better named free_.


good idea

Regards

Pavel


>
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)
>


Re: [HACKERS] Assignment of valid collation for SET operations on queries with UNKNOWN types.

2017-01-23 Thread Michael Paquier
On Tue, Jan 24, 2017 at 1:26 AM, Tom Lane  wrote:
> I wrote:
>> Ashutosh Bapat  writes:
>>> UNKNOWN is not exactly a pseudo-type.
>
>> Well, as I said to Michael just now, I think we should turn it into one
>> now that we're disallowing it in tables, because "cannot be used as a
>> table column" is more or less the definition of a pseudotype.
>
> I experimented with this, and it actually doesn't seem to be any harder
> than the attached: there's one type_sanity query that changes results,
> and otherwise all the regression tests pass.

Indeed.

> I've grepped the code for references to UNKNOWNOID and TYPTYPE_PSEUDO,
> and I can't find any places where the behavior would change in a way
> that we don't want.  Basically it looks like we'd disallow UNKNOWN as
> a domain base, a PL function argument or result, and a plpgsql local
> variable; and all of those seem like good things from here.

This has the merit to fix the ugly check in heap.c, so you may want to
merge both patches. At least I'd suggest to do so.

> Have not checked the docs.

Just looked at that.

As unknown is a pseudo type, I don't think you need
TYPCATEGORY_UNKNOWN in pg_type.h or even the mention to the unknown
type in catalogs.sgml as that becomes a pseudo-type.

The table of Pseudo-Types needs to be updated as well with unknown in
datatype.sgml.

For domains, it is still necessary to add an extra check in pg_upgrade
and fail the upgrade if one of the domains declared uses the type
unknown. Domains are not listed in pg_class, and are only present in
pg_type. If you don't do that, the binary restore would just fail.
-- 
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] parallelize queries containing subplans

2017-01-23 Thread Amit Kapila
On Thu, Jan 19, 2017 at 4:51 PM, Dilip Kumar  wrote:
> On Thu, Jan 19, 2017 at 3:05 PM, Dilip Kumar  wrote:
>> During debugging I found that subplan created for below part of the
>> query is parallel_unsafe, Is it a problem or there is some explanation
>> of why it's not parallel_safe,
>
> Okay, so basically we don't have any mechanism to perform parallel
> scan on CTE. And, IMHO subplan built for CTE (using SS_process_ctes)
> must come along with CTE scan. So I think we can avoid setting below
> code because we will never be able to test its side effect, another
> argument can be that if we don't consider the final effect, and just
> see this subplan then by logic it should be marked parallel-safe or
> unsafe as per it's path and it will not have any side effect, as it
> will finally become parallel-unsafe. So it's your call to keep it
> either way.
>

Yeah, actually setting parallel_safety information for subplan from
corresponding is okay.  However, in this particular case as we know
that it might not be of any use till we enable parallelism for CTE
Scan (and doing that is certainly not essential for this project).
So, I have set parallel_safe as false for CTE subplans.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


pq_pushdown_subplan_v4.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] contrib modules and relkind check

2017-01-23 Thread Amit Langote
Some contrib functions fail to fail sooner when relations of unsupported
relkinds are passed, resulting in error message like one below:

create table foo (a int);
create view foov as select * from foo;
select pg_visibility('foov', 0);
ERROR:  could not open file "base/13123/16488": No such file or directory

Attached patch fixes that for all such functions I could find in contrib.

It also installs RELKIND_PARTITIONED_TABLE as unsupported in a couple of
places (in pageinspect and pgstattuple).

Thanks,
Amit
>From 9324727c58f2debb45c7ecd6d620a3fe8b2a Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 24 Jan 2017 13:22:17 +0900
Subject: [PATCH] Add relkind checks to certain contrib modules

Contrib functions such a pg_visibility, pg_relpages are only applicable
to certain relkinds; error out if otherwise.

Also, RELKIND_PARTITIONED_TABLE was not added to the pageinspect's and
pgstattuple's list of unsupported relkinds.
---
 contrib/pageinspect/rawpage.c |  5 
 contrib/pg_visibility/pg_visibility.c | 28 ++
 contrib/pgstattuple/pgstatindex.c | 44 +++
 contrib/pgstattuple/pgstattuple.c |  3 +++
 4 files changed, 80 insertions(+)

diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index a2ac078d40..65aae6b6f8 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -121,6 +121,11 @@ get_raw_page_internal(text *relname, ForkNumber forknum, BlockNumber blkno)
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
  errmsg("cannot get raw page from foreign table \"%s\"",
 		RelationGetRelationName(rel;
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+		ereport(ERROR,
+(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot get raw page from partitioned table \"%s\"",
+		RelationGetRelationName(rel;
 
 	/*
 	 * Reject attempts to read non-local temporary relations; we would be
diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 5580637870..61d7224ee7 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -114,6 +114,15 @@ pg_visibility(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* Other relkinds don't have visibility info */
+	if (rel->rd_rel->relkind != RELKIND_RELATION &&
+		rel->rd_rel->relkind != RELKIND_MATVIEW &&
+		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("\"%s\" is not a table, materialized view, or TOAST table",
+		RelationGetRelationName(rel;
+
 	if (blkno < 0 || blkno > MaxBlockNumber)
 		ereport(ERROR,
 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -257,6 +266,16 @@ pg_visibility_map_summary(PG_FUNCTION_ARGS)
 	bool		nulls[2];
 
 	rel = relation_open(relid, AccessShareLock);
+
+	/* Other relkinds don't have visibility info */
+	if (rel->rd_rel->relkind != RELKIND_RELATION &&
+		rel->rd_rel->relkind != RELKIND_MATVIEW &&
+		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("\"%s\" is not a table, materialized view, or TOAST table",
+		RelationGetRelationName(rel;
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 
 	for (blkno = 0; blkno < nblocks; ++blkno)
@@ -464,6 +483,15 @@ collect_visibility_data(Oid relid, bool include_pd)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* Other relkinds don't have visibility info */
+	if (rel->rd_rel->relkind != RELKIND_RELATION &&
+		rel->rd_rel->relkind != RELKIND_MATVIEW &&
+		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("\"%s\" is not a table, materialized view, or TOAST table",
+		RelationGetRelationName(rel;
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 	info = palloc0(offsetof(vbits, bits) +nblocks);
 	info->next = 0;
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index b40669250a..e9e2dc2207 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -362,6 +362,17 @@ pg_relpages(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* only the following relkinds have storage */
+	if (!(rel->rd_rel->relkind == RELKIND_RELATION ||
+		  rel->rd_rel->relkind == RELKIND_INDEX ||
+		  rel->rd_rel->relkind == RELKIND_MATVIEW ||
+		  rel->rd_rel->relkind == RELKIND_SEQUENCE ||
+		  rel->rd_rel->relkind == RELKIND_TOASTVALUE))
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("\"%s\" is not a table, index, materialized view, sequence, or TOAST table",
+		RelationGetRelationName(rel;
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -383,6 +394,17 @@ pg_relpages_v1_5(PG_FUNCTION_ARGS)
 	

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

2017-01-23 Thread Dilip Kumar
On Wed, Jan 18, 2017 at 10:45 AM, Haribabu Kommi
 wrote:
> The use case for this view is to find out the number of different SQL
> statements
> that are getting executed successfully on an instance by the
> application/user.
>
>> I have few comments/questions.
>>
>> 
>> If you execute the query with EXECUTE then commandTag will be EXECUTE
>> that way we will not show the actual query type, I mean all the
>> statements will get the common tag "EXECUTE".
>>
>> You can try this.
>> PREPARE fooplan (int) AS INSERT INTO t VALUES($1);
>> EXECUTE fooplan(1);
>>
>> --
>
>
> Yes, that's correct. Currently the count is incremented based on the base
> command,
> because of this reason, the EXECUTE is increased, instead of the actual
> operation.

Okay, As per your explaination, and Robert also pointed out that it
can be a feature for someone and bug for others. I would have
preferred it shows actual SQL statement. But I have no objection to
what you are doing.
>
>
>>
>> + /* Destroy the SQL statement counter stats HashTable */
>> + hash_destroy(pgstat_sql);
>> +
>> + /* Create SQL statement counter Stats HashTable */
>>
>> I think in the patch we have used mixed of "Stats HashTable" and
>> "stats HashTable", I think better
>> to be consistent throughout the patch. Check other similar instances.
>>
>> 
>
>
> Corrected.
>
>>
>>
>> @@ -1102,6 +1102,12 @@ exec_simple_query(const char *query_string)
>>
>>   PortalDrop(portal, false);
>>
>> + /*
>> + * Count SQL statement for pg_stat_sql view
>> + */
>> + if (pgstat_track_sql)
>> + pgstat_count_sqlstmt(commandTag);
>>
>> We are only counting the successful SQL statement, Is that intentional?
>
>
> Yes, I thought of counting the successful SQL operations that changed the
> database over a period of time. I am not sure whether there will be many
> failure operations that can occur on an instance to be counted.

Okay..
>
>>
>> --
>> I have noticed that pgstat_count_sqlstmt is called from
>> exec_simple_query and exec_execute_message. Don't we want to track the
>> statement executed from functions?
>> ---
> The view is currently designed to count user/application initiated SQL
> statements,
> but not the internal SQL queries that are getting executed from functions
> and etc.

Okay..
>
>>>+void
>>>+pgstat_count_sqlstmt(const char *commandTag)
>>>+{
>>>+ PgStat_SqlstmtEntry *htabent;
>>>+ bool found;
>>>+
>>>+ if (!pgstat_track_sql)
>>>+ return
>>
>>Callers of pgstat_count_sqlstmt are always ensuring that
>>pgstat_track_sql is true, seems it's redundant here.
>
> Removed the check in pgstat_count_sqlstmt statement and add it
> exec_execute_message
> function where the check is missed.
>
> Updated patch attached.

Overall patch looks fine to me and marking it "ready for committer".

There is two design decision, which I leave it for committer's decision.

1.  EXECUTE statement should show only as EXECUTE count, not the
actual SQL query.
2.  should we count statement execute from Functions or only statement
what is executed directly by the user as it's doing now?


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] macaddr 64 bit (EUI-64) datatype support

2017-01-23 Thread Haribabu Kommi
On Mon, Jan 23, 2017 at 6:29 PM, Kuntal Ghosh 
wrote:

> On Thu, Jan 19, 2017 at 7:46 AM, Haribabu Kommi
>  wrote:
> >
> > Updated patch attached.
> >
> I've looked at the updated patch. There are still some changes that
> needs to be done. It includes:
>

Thanks for the review.

1. Adding macaddr8 support for btree_gist and testcases.
> 2. Implementation of macaddr8 support for btree_gin is incomplete. You
> need to modify contrib/btree_gin/*.sql files as well. Also, testcases
> needs to be added.
>

Added the support for macaddr8 datatype for both btree_gin and btree_gist
modules and the tests also.

I didn't update the *.sql files to the latest version and just added the
update *.sql
file only as like earlier changes on btree_gist for UUID support to make it
easier
for the reviewer.

The patch is split into two parts.
1. Macaddr8 datatype support
2. Contrib module support.

Regards,
Hari Babu
Fujitsu Australia


mac_eui64_support_7.patch
Description: Binary data


contrib_macaddr8_1.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] Parallel bitmap heap scan

2017-01-23 Thread Dilip Kumar
On Mon, Jan 23, 2017 at 1:52 PM, Haribabu Kommi
 wrote:
> I reviewed 0002-hash-support-alloc-free-v12.patch, some minor comments.
>
> - SH_TYPE*tb;
> - uint64 size;
> + SH_TYPE *tb;
> + uint64 size;
>
> The above change may not be required.
>
> + if (tb->alloc)
> + {
> + tb->alloc->HashFree(tb->data, tb->alloc->args);
> + pfree(tb->alloc);
> + }
>
> The above code tries to free the tb->alloc memory. In case if the user
> has provide the alloc structure to SH_CREATE function and the same
> pointer is stored in the tb structure. And in free function freeing that
> memory may cause problem.
>
> So either explicitly mentioning that the input must a palloc'ed data or
> by default allocate memory and copy the input data into allocated
> memory.

I have changed as per the comments. 0002 and 0003 are changed, 0001 is
still the same.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


0001-opt-parallelcost-refactoring-v13.patch
Description: Binary data


0002-hash-support-alloc-free-v13.patch
Description: Binary data


0003-parallel-bitmap-heap-scan-v13.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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

2017-01-23 Thread Michael Paquier
On Tue, Jan 24, 2017 at 3:59 AM, Stephen Frost  wrote:
> I agree that we could probably just go ahead and switch over to starting
> on the clog changes (there was agreement somewhere about the new name
> for that too), but, well, if I was someone watching all of this
> discussion, I have to admit I probably wouldn't be too excited starting
> on another set of name changes with all of this going on.  Admittedly,
> the clog rename is a lot less user-facing and perhaps we should have
> started with it, but this is where we're at now.

There are no SQL-level functions and no binaries using the naming clog
or subtrans, so that's a good anti-complain start.
-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-01-23 Thread Ashutosh Sharma
> I've looked at the patch. It looks good. However, I was wondering why
> an exclusive lock for extension is necessary for reading the number
> blocks in this case. Please refer to the following code.
>
> +   /* Get the current relation length */
> +   LockRelationForExtension(rel, ExclusiveLock);
> +   nblocks = RelationGetNumberOfBlocks(rel);
> +   UnlockRelationForExtension(rel, ExclusiveLock);
>
> Apart from this, I've nothing else to add.


Actually in my case I may not need to acquire ExclusiveLock just to
get the number of pages in a relation. Infact I have already opened a
relation using
'AccessShareLock' which should be enough to read a table length.
Thanks for putting this point. I have corrected it in the attached v5
patch. I think
in 'pgstat_index()' currently LockRelationForExtension(rel,
ExclusiveLock); is being used just to read a table length which I feel
is not required. You may raise this point in the community in a
separete mail thread if you agree with it. Also, I guess
LockRelationForExtension() is used when trying to add a new page in an
existing relation so not sure if it would be right to call it from
contrib modules like pgstattuple where we are just trying to read the
tables.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com


0001-Add-pgstathashindex-to-pgstattuple-extension-v5.patch
Description: invalid/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] Faster methods for getting SPI results (460% improvement)

2017-01-23 Thread Craig Ringer
On 24 January 2017 at 11:23, Jim Nasby  wrote:

> I finally got all the kinks worked out and did some testing with python 3.
> Performance for my test [1] improved ~460% when returning a dict of lists
> (as opposed to the current list of dicts). Based on previous testing, I
> expect that using this method to return a list of dicts will be about 8%
> slower. The inconsistency in results on 2.7 has to do with how python 2
> handles ints.

Impressive results.

> I think the last step here is to figure out how to support switching between
> the current behavior and the "columnar" behavior of a dict of lists.

That sounds like it'd be much better approached as a separate, later patch.

If I understand you correctly, you propose to return the resultset

a   b
1  10
2  20

which is currently returned as

[ {"a":1, "b":10}, {"a":2, "b":20} ]

instead as

{ "a": [1, 2], "b": [10, 20] }

?

If so I see that as a lot more of a niche thing. I can see why it'd be
useful and would help performance, but it seems much more disruptive.
It requires users to discover it exists, actively adopt a different
style of ingesting data, etc. For a 10%-ish gain in a PL.

I strongly suggest making this design effort a separate thread, and
focusing on the SPI improvements that give "free" no-user-action
performance boosts here.


-- 
 Craig Ringer   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] Contrib: pqasyncnotifier.c -- a shell command client for LISTEN

2017-01-23 Thread Nico Williams
I should also note that this is on github at
https://github.com/twosigma/postgresql-contrib

Nico
-- 


-- 
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] Contrib: alternative MATERIALIZED VIEWs

2017-01-23 Thread Nico Williams
On Mon, Jan 23, 2017 at 06:05:25PM -0600, Jim Nasby wrote:
> On 1/23/17 5:38 PM, Nico Williams wrote:
> >Attached is an alternative implementation of MATERIALIZED VIEWs.
> 
> Interesting. I don't see this being accepted into core because it's plpgsql
> and it depends on the user to track what criteria to use to apply the
> update. The second item is the biggest issue.

I myself said this is not properly integrated.  I do use this in an
actual system, but ideally if any of this is to be welcomed, then it'd
have to be properly integrated.  I don't see what's wrong with the use
of plpgsql as the MV system in PG uses SQL, but in a proper integration
a lot of this would be re-written in C (the use of SQL for the delta
computation and the updates of the history table would remain, but I
think too so would the triggers needed to update the history table when
the MV is updated directly).

As to the second issue...  Just the other day Kevin Grittner was
concerned about automatic MV updates because some of them can take too
long.  Now, PG could timeout MV updates, roll them back, and mark an MV
as requiring a refresh, but the user might still have something to say
about this: they really, really might want some updates to always happen
synchronously, and others always asynchronously.  I think it's really
necessary to give the user this sort of control.

> That said, I think this would be useful to some people as an extension. I
> suggest you put it on github (or equivalent) and upload it to
> http://pgxn.org.

Ah, I forgot to mention that it is on github here:

https://github.com/twosigma/postgresql-contrib

> In terms of community support, the next step is to get statement-level
> support for NEW and OLD, something I think Kevin has been working on.

Well, I think there's a lot to look at here.  That's partly why a
plpgsql-coded implementation is a good first step, IMO: it helps find
issues.  For me the need for a PK for MVs is fairly important; but the need
for deltas/history table is critical, and there is no reason not to have
it considering that deltas are produced internally during REFRESH ..
CONCURRENTLY.

Nico
-- 


-- 
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] Contrib: pqasyncnotifier.c -- a shell command client for LISTEN

2017-01-23 Thread Nico Williams
On Tue, Jan 24, 2017 at 12:48:49AM +0100, Marko Tiikkaja wrote:
> Did you forget the attachment?

I guess I must have.  Attached this time.
/*
 * Copyright (c) 2016 Two Sigma Open Source, LLC.
 * All Rights Reserved
 *
 * Permission to use, copy, modify, and distribute this software and its
 * documentation for any purpose, without fee, and without a written agreement
 * is hereby granted, provided that the above copyright notice and this
 * paragraph and the following two paragraphs appear in all copies.
 *
 * IN NO EVENT SHALL TWO SIGMA OPEN SOURCE, LLC BE LIABLE TO ANY PARTY FOR
 * DIRECT, INDIRECT, SPECIAL, INCIDENTAL, OR CONSEQUENTIAL DAMAGES, INCLUDING
 * LOST PROFITS, ARISING OUT OF THE USE OF THIS SOFTWARE AND ITS DOCUMENTATION,
 * EVEN IF TWO SIGMA OPEN SOURCE, LLC HAS BEEN ADVISED OF THE POSSIBILITY OF
 * SUCH DAMAGE.
 *
 * TWO SIGMA OPEN SOURCE, LLC SPECIFICALLY DISCLAIMS ANY WARRANTIES, INCLUDING,
 * BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
 * FOR A PARTICULAR PURPOSE. THE SOFTWARE PROVIDED HEREUNDER IS ON AN "AS IS"
 * BASIS, AND TWO SIGMA OPEN SOURCE, LLC HAS NO OBLIGATIONS TO PROVIDE
 * MAINTENANCE, SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS.
 */

/*
 * Based on src/test/examples/testlibpq2.c from Postgres 9.4.4
 *
 * pqasyncnotifier - LISTENs and reports notifications without polling
 *
 * Usage: pqasyncnotifier CONNINFO TABLE_NAME
 */

#include 
#ifdef WIN32
#include 
#endif

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

static void
err(PGconn *conn, ConnStatusType rcc, ExecStatusType rce, const char *msg)
{
int e = errno;

fprintf(stderr, "Error: %s: ", msg);
if (conn == NULL || (rcc == CONNECTION_OK && rce == PGRES_COMMAND_OK))
fprintf(stderr, "%s", strerror(e));
else if (conn != NULL && (rcc != CONNECTION_OK || rce != PGRES_COMMAND_OK))
fprintf(stderr, "%s", PQerrorMessage(conn));
else
fprintf(stderr, "Unknown error");
fprintf(stderr, "\n");
if (conn)
PQfinish(conn);
exit(e ? e : 1);
}

static struct option lopts[] = {
{"help", 0, 0, 'h'},
{"verbose", 0, 0, 'v'},
{"daemonize", 0, 0, 'D'},
{"include-payload", 0, 0, 'd'},
{"include-pid", 0, 0, 'p'},
{"include-time", 0, 0, 't'},
{"include-channel-name", 0, 0, 'c'},
};

static void
usage(const char *prog, int e)
{
size_t i;

if (strrchr(prog, '/') != NULL && strrchr(prog, '/')[1] != '\0')
prog = strrchr(prog, '/') + 1;

fprintf(stderr, "Usage: %s [options] CONNINFO TABLE-NAME ...\n", prog);
fprintf(stderr, "Options:\n");
for (i = 0; i < sizeof(lopts)/sizeof(lopts[0]); i++)
fprintf(stderr, "\t-%c, --%s\n", lopts[i].val, lopts[i].name);
exit(e);
}

static
void
daemonize(int ready)
{
static int pipe_fds[2] = {-1, -1};
pid_t pid;
char code = 1;

if (ready) {
errno = 0;
if (pipe_fds[1] != -1) {
while (write(pipe_fds[1], "", sizeof("")) != 1 && errno != EINTR)
;
if (errno != 0)
err(NULL, 0, 0, "write() failed while daemonizing");
if (close(pipe_fds[1]) != 0)
err(NULL, 0, 0, "close() failed while daemonizing");
pipe_fds[1] = -1;
}
printf("READY: %jd\n", (intmax_t)getpid());
return;
}

if (pipe(pipe_fds) == -1)
err(NULL, 0, 0, "pipe() failed");
pid = fork();
if (pid == -1)
err(NULL, 0, 0, "fork() failed");
if (pid == 0) {
(void) close(pipe_fds[0]);
pipe_fds[0] = -1;
return;
}
(void) close(pipe_fds[1]);
pipe_fds[1] = -1;
while (read(pipe_fds[0], , sizeof(code)) != 1 && errno != EINTR)
;
_exit(code);
}

int
main(int argc, char **argv)
{
const char  *prog = argv[0];
const char  *conninfo;
PGconn  *conn = NULL;
PGresult*res;
PGnotify*notify;
ConnStatusType  rcc = CONNECTION_OK;
ExecStatusType  rce = PGRES_COMMAND_OK;
int include_relname = 0;
int include_payload = 0;
int include_pid = 0;
int include_time = 0;
int verbose = 0;
charc;

setlinebuf(stdout);

while ((c = getopt_long(argc, argv, "+Dcdhptv", lopts, NULL)) != -1) {
switch (c) {
case 'D':
daemonize(0);
case 'c':
include_relname = 1;
break;
case 'd':
include_payload = 1;
break;
case 'h':
usage(prog, 0);
break;
case 'p':
include_pid = 1;
break;
case 't':
include_time = 1;
break;
case 'v':
verbose = 1;
break;
default:
usage(prog, 1);
break;
}
}

argc 

Re: [HACKERS] src/test/README for logical replication

2017-01-23 Thread Fujii Masao
On Tue, Jan 24, 2017 at 7:17 AM, Craig Ringer  wrote:
> src/test/README wasn't updated for the new src/test/subscription entry.
>
> Patch attached.

Thanks! Pushed.

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] patch proposal

2017-01-23 Thread Michael Paquier
On Tue, Jan 24, 2017 at 7:22 AM, David Steele  wrote:
> 3) There are no tests.  I believe that
> src/test/recovery/t/006_logical_decoding.pl would be the best place to
> insert your tests.

003_recovery_targets.pl is the test for recovery targets. The routines
it has would be helpful.

> I'm marking this as "Waiting on Author", but honestly I think "Returned
> with Feedback" might be more appropriate.  I'll leave that up to the CFM.

We could wait a couple of days before that, this patch just got a
review. (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] Checksums by default?

2017-01-23 Thread Tom Lane
Merlin Moncure  writes:
> Hm, but at least in some cases wouldn't it protect people from further
> damage?  End user data damage ought to prevented at all costs IMO.

Well ... not directly.  Disallowing you from accessing busted block A
doesn't in itself prevent the same thing from happening to block B.
The argument seems to be that checksum failure complaints might prompt
users to, say, replace a failing disk drive before it goes dead completely.
But I think there's a whole lot of wishful thinking in that, particularly
when it comes to the sort of low-information users who would actually
be affected by a change in the default checksum setting.

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] GSoC 2017

2017-01-23 Thread Peter van Hardenberg
On Mon, Jan 23, 2017 at 4:12 PM, Jim Nasby  wrote:

> On 1/23/17 3:45 PM, Peter van Hardenberg wrote:
>
>> A new currency type would be nice, and if kept small in scope, might be
>> manageable.
>>
>
> I'd be rather nervous about this. My impression of community consensus on
> this is a currency type that doesn't somehow support conversion between
> different currencies is pretty useless, and supporting conversions opens a
> 55 gallon drum of worms. I could certainly be mistaken in my impression,
> but I think there'd need to be some kind of consensus on what a currency
> type should do before putting that up for GSoC.
>

There's a relatively simple solution to the currency conversion problem
which avoids running afoul of the various mistakes some previous
implementations have made. Track currencies separately and always ask for a
conversion chart at operation time.

Let the user specify the values they want at conversion time. That looks
like this:

=> select '1 CAD'::currency + '1 USD'::currency + '1 CHF'::currency
'1.00CAD 1.00USD 1.00CHF'

=> select convert('10.00CAD'::new_currency, ('USD, '1.25', 'CHF',
'1.50')::array, 'USD')
12.50USD

The basic concept is that the value of a currency type is that it would
allow you to operate in multiple currencies without accidentally adding
them. You'd flatten them to a single type if when and how you wanted for
any given operation but could work without fear of losing information.

I have no opinion about the most pleasing notation for the currency
conversion chart, but I imagine it would be reasonable to let users provide
a default set of conversion values somewhere.

There are interesting and worthwhile conversations to have about
non-decimal currencies, but I think it would be totally reasonable not to
support them at all in a first release. As for currency precision, I would
probably consider leaning on numeric under the hood for the actual currency
values themselves but IANAA (though I have done quite a lot of work on
billing systems).

If it would be helpful, I could provide a detailed proposal on the wiki for
others to critique?

-
Peter van Hardenberg
San Francisco, California
"Everything was beautiful, and nothing hurt."—Kurt Vonnegut


Re: [HACKERS] Faster methods for getting SPI results (460% improvement)

2017-01-23 Thread Jim Nasby

On 1/5/17 9:50 PM, Jim Nasby wrote:

The * on that is there's something odd going on where plpython starts
out really fast at this, then gets 100% slower. I've reached out to some
python folks about that. Even so, the overall results from a quick test
on my laptop are (IMHO) impressive:

Old CodeNew CodeImprovement
Pure SQL2 sec  2 sec
plpython12.7-14 sec4-10 sec ~1.3-3x
plpython - SQL 10.7-12 sec 2-8 sec  ~1.3-6x

Pure SQL is how long an equivalent query takes to run with just SQL.
plpython - SQL is simply the raw python times minus the pure SQL time.


I finally got all the kinks worked out and did some testing with python 
3. Performance for my test [1] improved ~460% when returning a dict of 
lists (as opposed to the current list of dicts). Based on previous 
testing, I expect that using this method to return a list of dicts will 
be about 8% slower. The inconsistency in results on 2.7 has to do with 
how python 2 handles ints.


Someone who's familiar with pl/perl should take a look at this and see 
if it would apply there. I've attached the SPI portion of this patch.


I think the last step here is to figure out how to support switching 
between the current behavior and the "columnar" behavior of a dict of 
lists. I believe the best way to do that is to add two optional 
arguments to the execution functions: container=[] and members={}, and 
then copy those to produce the output objects. That means you can get 
the new behavior by doing something like:


plpy.execute('...', container={}, members=[])

Or, more interesting, you could do:

plpy.execute('...', container=Pandas.DataFrame, members=Pandas.Series)

since that's what a lot of people are going to want anyway.

In the future we could also add a GUC to change the default behavior.

Any concerns with that approach?

1:

d = plpy.execute('SELECT s AS some_table_id, s AS some_field_name, s AS 
some_other_field_name FROM generate_series(1,{}) s'.format(iter) )
return len(d['some_table_id'])

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 7bd37283b7..97585d272e 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -55,7 +55,8 @@ static void _SPI_prepare_oneshot_plan(const char *src, 
SPIPlanPtr plan);
 
 static int _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
  Snapshot snapshot, Snapshot 
crosscheck_snapshot,
- bool read_only, bool fire_triggers, uint64 
tcount);
+ bool read_only, bool fire_triggers, uint64 
tcount,
+ DestReceiver *callback);
 
 static ParamListInfo _SPI_convert_params(int nargs, Oid *argtypes,
Datum *Values, const char *Nulls);
@@ -320,7 +321,34 @@ SPI_execute(const char *src, bool read_only, long tcount)
 
res = _SPI_execute_plan(, NULL,
InvalidSnapshot, 
InvalidSnapshot,
-   read_only, true, 
tcount);
+   read_only, true, 
tcount, NULL);
+
+   _SPI_end_call(true);
+   return res;
+}
+int
+SPI_execute_callback(const char *src, bool read_only, long tcount,
+   DestReceiver *callback)
+{
+   _SPI_plan   plan;
+   int res;
+
+   if (src == NULL || tcount < 0)
+   return SPI_ERROR_ARGUMENT;
+
+   res = _SPI_begin_call(true);
+   if (res < 0)
+   return res;
+
+   memset(, 0, sizeof(_SPI_plan));
+   plan.magic = _SPI_PLAN_MAGIC;
+   plan.cursor_options = 0;
+
+   _SPI_prepare_oneshot_plan(src, );
+
+   res = _SPI_execute_plan(, NULL,
+   InvalidSnapshot, 
InvalidSnapshot,
+   read_only, true, 
tcount, callback);
 
_SPI_end_call(true);
return res;
@@ -354,7 +382,34 @@ SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const 
char *Nulls,

_SPI_convert_params(plan->nargs, plan->argtypes,

Values, Nulls),
InvalidSnapshot, 
InvalidSnapshot,
-   read_only, true, 
tcount);
+   read_only, true, 
tcount, NULL);
+
+   _SPI_end_call(true);
+   return res;
+}
+
+/* Execute a previously prepared plan with a callback Destination */
+int

Re: [HACKERS] Checksums by default?

2017-01-23 Thread Merlin Moncure
On Mon, Jan 23, 2017 at 8:07 PM, Tom Lane  wrote:
> Peter Geoghegan  writes:
>> I thought that checksums went in in part because we thought that there
>> was some chance that they'd find bugs in Postgres.
>
> Not really.  AFAICS the only point is to catch storage-system malfeasance.
>
> It's barely possible that checksumming would help detect cases where
> we'd written data meant for block A into block B, but I don't rate
> that as being significantly more probable than bugs in the checksum
> code itself.  Also, if that case did happen, the checksum code might
> "detect" it in some sense, but it would be remarkably unhelpful at
> identifying the actual cause.

Hm, but at least in some cases wouldn't it protect people from further
damage?  End user data damage ought to prevented at all costs IMO.

merlin


-- 
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] Checksums by default?

2017-01-23 Thread Andres Freund
On 2017-01-23 21:40:53 -0500, Stephen Frost wrote:
> Perhaps I'm missing something here, but with checksums enabled, a hint
> bit update is going to dirty the page (and we're going to write it into
> the WAL and write it out to the heap), no?

No.  We only WAL log hint bits the first time a page is modified after a
checkpoint.  It's quite likely that you'll set hint bits in the same
checkpoint cycle as the row has been modified last (necessating the hint
bit change).  So we can't just pessimize this.

I'm a bit confused about the amount of technically wrong arguments in
this thread.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Checksums by default?

2017-01-23 Thread Tom Lane
Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> But we don't maintain the checksum of a page while it sits in shared
>> buffers.  Trying to do so would break, eg, concurrent hint-bit updates.

> Hence why I said 'clean' pages..

When we write out a page, we copy it into private memory and compute the
checksum there, right?  We don't copy the checksum back into the page's
shared-buffer image, and if we did, that would defeat the point because we
would've had to maintain exclusive lock on the buffer or else the checksum
might be out of date.  So I don't see how this works without throwing away
a lot of carefully-designed behavior.

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] Online enabling of page level checksums

2017-01-23 Thread Jim Nasby

On 1/23/17 12:03 PM, Greg Stark wrote:

On Jan 22, 2017 11:13 AM, "Magnus Hagander" > wrote:


Yes, this means the entire db will end up in the transaction log
since everything is rewritten. That's not great, but for a lot of
people that will be a trade they're willing to make since it's a
one-time thing. Yes, this background process might take days or
weeks - that's OK as long as it happens online.


I'm not sure that's actually necessary. You could just log a wal record
saying "checksum this block" and if it gets replayed then recalculate
the checksum on that block again. This record could be exempt from the
usual rules for having a fpw.

There's no danger of torn pages from the checksum alone. The danger
would be if some other operation does dirty that page then your need to
know that the page is in this weird in between state where it's dirty
but not yet had a fpw written.

I'm not sure whether it's worth the infrastructure to have such a state
just for this or not. On the other hand it sounds like something that
would be useful.


I'm a bit concerned about how much fancy we're trying to put into a 
first pass at this.


I think it's reasonable to require a fairly significant amount of effort 
on the part of an admin to enable checksums. For that matter, I think 
it'd be a significant improvement if there was NO way to enable 
checksums, only disable them. That would at least allow us to enable 
them by default in initdb and provide DBAs an easy way to disable them 
if desired. That would get us a lot more data about whether checksums 
help with corruption than we're going to get otherwise.


To put it another way, I think it's entirely reasonable *from a 
technical standpoint* to enable by default in 10, with only the ability 
to dynamically disable. Given the concerns that keep popping up about 
dynamically enabling, I'm not at all sure that we could get that into 10.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Checksums by default?

2017-01-23 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Jim Nasby  writes:
> > On 1/23/17 7:47 PM, Stephen Frost wrote:
> >> It might be interesting to consider checking them in 'clean' pages in
> >> shared_buffers in a background process, as that, presumably, *would*
> >> detect shared buffers corruption.
> 
> > Hmm... that would be interesting. Assuming the necessary functions are 
> > exposed it presumably wouldn't be difficult to do that in an extension, 
> > as a bgworker.
> 
> But we don't maintain the checksum of a page while it sits in shared
> buffers.  Trying to do so would break, eg, concurrent hint-bit updates.

Hence why I said 'clean' pages..

Perhaps I'm missing something here, but with checksums enabled, a hint
bit update is going to dirty the page (and we're going to write it into
the WAL and write it out to the heap), no?

We'd have to accept that checking the checksum on a page would require a
read lock on each page as it goes through, I imagine, though we could do
something like check if the page is clean, obtain a read lock, then
check if it's still clean and if so calculate the checksum and then let
the lock go, then at least we're avoiding trying to lock dirty pages.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Checksums by default?

2017-01-23 Thread Jim Nasby

On 1/23/17 8:24 PM, Tom Lane wrote:

Jim Nasby  writes:

On 1/23/17 7:47 PM, Stephen Frost wrote:

It might be interesting to consider checking them in 'clean' pages in
shared_buffers in a background process, as that, presumably, *would*
detect shared buffers corruption.



Hmm... that would be interesting. Assuming the necessary functions are
exposed it presumably wouldn't be difficult to do that in an extension,
as a bgworker.


But we don't maintain the checksum of a page while it sits in shared
buffers.  Trying to do so would break, eg, concurrent hint-bit updates.


Hrm, I thought the checksum would be valid if the buffer is marked clean?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Checksums by default?

2017-01-23 Thread Tom Lane
Jim Nasby  writes:
> On 1/23/17 7:47 PM, Stephen Frost wrote:
>> It might be interesting to consider checking them in 'clean' pages in
>> shared_buffers in a background process, as that, presumably, *would*
>> detect shared buffers corruption.

> Hmm... that would be interesting. Assuming the necessary functions are 
> exposed it presumably wouldn't be difficult to do that in an extension, 
> as a bgworker.

But we don't maintain the checksum of a page while it sits in shared
buffers.  Trying to do so would break, eg, concurrent hint-bit updates.

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] Checksums by default?

2017-01-23 Thread Jim Nasby

On 1/23/17 7:47 PM, Stephen Frost wrote:

It might be interesting to consider checking them in 'clean' pages in
shared_buffers in a background process, as that, presumably, *would*
detect shared buffers corruption.


Hmm... that would be interesting. Assuming the necessary functions are 
exposed it presumably wouldn't be difficult to do that in an extension, 
as a bgworker.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] COPY IN/BOTH vs. extended query mode

2017-01-23 Thread Robert Haas
According to the documentation for COPY IN mode, "If the COPY command
was issued via an extended-query message, the backend will now discard
frontend messages until a Sync message is received, then it will issue
ReadyForQuery and return to normal processing."  I added a similar
note to the documentation for COPY BOTH mode in
91fa8532f4053468acc08534a6aac516ccde47b7, and the documentation
accurately describes the behavior of the server.  However, this seems
to make fully correct error handling for clients using libpq almost
impossible, because PQsendQueryGuts() sends
Parse-Bind-Describe-Execute-Sync in one shot without regard to whether
the command that was just sent invoked COPY mode (cf. the note in
CopyGetData about why we ignore Flush and Sync in that function).

So imagine that the client uses libpq to send (via the extended query
protocol) a COPY IN command (or some hypothetical command that starts
COPY BOTH mode to begin).  If the server throws an error before the
Sync message is consumed, it will bounce back to PostgresMain which
will set doing_extended_query_message = true after which it will
consume messages, find the Sync, reset that flag, and send
ReadyForQuery.  On the other hand, if the server enters CopyBoth mode,
consumes the Sync message in CopyGetData (or a similar function), and
*then* throws an ERROR, the server will wait for a second Sync message
from the client before issuing ReadyForQuery.  There is no sensible
way of coping with this problem in libpq, because there is no way for
the client to know which part of the server code consumed the Sync
message that it already sent.  In short, from the client's point of
view, if it enters COPY IN or COPY BOTH mode via the extend query
protocol, and an error occurs on the server, the server MAY OR MAY NOT
expect a further Sync message before issuing ReadyForQuery, and the
client has no way of knowing -- except maybe waiting for a while to
see what happens.

It does not appear to me that there is any good solution to this
problem.  Fixing it on the server side would require a wire protocol
change - e.g. one kind of Sync message that is used in a
Parse-Bind-Describe-Execute-Sync sequence that only terminates
non-COPY commands and another kind that is used to signal the end even
of COPY.  Fixing it on the client side would require all clients to
know prior to initiating an extended-query-protocol sequence whether
or not the command was going to initiate COPY, which is an awful API
even if didn't constitute an impossible-to-contemplate backward
compatibility break.  Perhaps we will have to be content to document
the fact that this part of the protocol is depressingly broken...

...unless of course somebody can see something that I'm missing here
and the situation isn't as bad as it currently appears to me to be.

-- 
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] Checksums by default?

2017-01-23 Thread Tom Lane
Peter Geoghegan  writes:
> I thought that checksums went in in part because we thought that there
> was some chance that they'd find bugs in Postgres.

Not really.  AFAICS the only point is to catch storage-system malfeasance.

It's barely possible that checksumming would help detect cases where
we'd written data meant for block A into block B, but I don't rate
that as being significantly more probable than bugs in the checksum
code itself.  Also, if that case did happen, the checksum code might
"detect" it in some sense, but it would be remarkably unhelpful at
identifying the actual cause.

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] Checksums by default?

2017-01-23 Thread Peter Geoghegan
On Mon, Jan 23, 2017 at 6:01 PM, Tom Lane  wrote:
> Maybe this is a terminology problem.  I'm taking "false positive" to mean
> "checksum reports a failure, but in fact there is no observable data
> corruption".  Depending on why the false positive occurred, that might
> help alert you to underlying storage problems, but it isn't helping you
> with respect to being able to access your perfectly valid data.

It was a terminology problem. Thank you for the clarification.


-- 
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] Checksums by default?

2017-01-23 Thread Tom Lane
Peter Geoghegan  writes:
> Perhaps I've missed the point entirely, but, I have to ask: How could
> there ever be false positives?

Bugs.  For example, checksum is computed while somebody else is setting
a hint bit in the page, so that what is written out is completely valid
except that the checksum doesn't match.  (I realize that that specific
scenario should be impossible given our implementation, but I hope you
aren't going to claim that bugs in the checksum code are impossible.)

Maybe this is a terminology problem.  I'm taking "false positive" to mean
"checksum reports a failure, but in fact there is no observable data
corruption".  Depending on why the false positive occurred, that might
help alert you to underlying storage problems, but it isn't helping you
with respect to being able to access your perfectly valid data.

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] Checksums by default?

2017-01-23 Thread Peter Geoghegan
On Mon, Jan 23, 2017 at 5:47 PM, Stephen Frost  wrote:
>> Perhaps I've missed the point entirely, but, I have to ask: How could
>> there ever be false positives? With checksums, false positives are
>> simply not allowed. Therefore, there cannot be a false positive,
>> unless we define checksums as a mechanism that should only find
>> problems that originate somewhere at or below the filesystem. We
>> clearly have not done that, so ISTM that checksums could legitimately
>> find bugs in the checksum code. I am not being facetious.
>
> I'm not sure I'm following your question here.  A false positive would
> be a case where the checksum code throws an error on a page whose
> checksum is correct, or where the checksum has failed but nothing is
> actually wrong/different on the page.

I thought that checksums went in in part because we thought that there
was some chance that they'd find bugs in Postgres. I was under the
impression that that was at least a secondary goal of checksums.

> As for the purpose of checksums, it's exactly to identify cases where
> the page has been changed since we wrote it out, due to corruption in
> the kernel, filesystem, storage system, etc.  As we only check them when
> we read in a page and calculate them when we go to write the page out,
> they aren't helpful for shared_buffers corruption, generally speaking.

I'd have guessed that they might catch a bug in recovery itself, even
when the filesystem maintains the guarantees Postgres requires.

In any case, it seems exceedingly unlikely that the checksum code
itself would fail.

-- 
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] Checksums by default?

2017-01-23 Thread Petr Jelinek
On 24/01/17 02:39, Michael Paquier wrote:
> On Tue, Jan 24, 2017 at 10:26 AM, Stephen Frost  wrote:
>> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> I don't recall ever seeing a checksum failure on a Heroku Postgres
> database,
>>
>> Not sure how this part of that sentence was missed:
>>
>> -
>> ... even though they were enabled as soon as the feature became
>> available.
>> -
>>
>> Which would seem to me to say "the code's been running for a long time
>> on a *lot* of systems without throwing a false positive or surfacing a
>> bug."
> 
> I am reading that similarly to what Tom is seeing: enabling it has
> proved Heroku that it did not catch problems in years, meaning that
> the performance cost induced by enabling it has paid nothing in
> practive, except the insurance to catch up problems should they
> happen.
> 

+1

-- 
  Petr Jelinek  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] Checksums by default?

2017-01-23 Thread Stephen Frost
* Peter Geoghegan (p...@heroku.com) wrote:
> On Mon, Jan 23, 2017 at 5:26 PM, Stephen Frost  wrote:
> > Not sure how this part of that sentence was missed:
> >
> > -
> > ... even though they were enabled as soon as the feature became
> > available.
> > -
> >
> > Which would seem to me to say "the code's been running for a long time
> > on a *lot* of systems without throwing a false positive or surfacing a
> > bug."
> 
> I think you've both understood what I said correctly. Note that I
> remain neutral on the question of whether or not checksums should be
> enabled by default.
> 
> Perhaps I've missed the point entirely, but, I have to ask: How could
> there ever be false positives? With checksums, false positives are
> simply not allowed. Therefore, there cannot be a false positive,
> unless we define checksums as a mechanism that should only find
> problems that originate somewhere at or below the filesystem. We
> clearly have not done that, so ISTM that checksums could legitimately
> find bugs in the checksum code. I am not being facetious.

I'm not sure I'm following your question here.  A false positive would
be a case where the checksum code throws an error on a page whose
checksum is correct, or where the checksum has failed but nothing is
actually wrong/different on the page.

As for the purpose of checksums, it's exactly to identify cases where
the page has been changed since we wrote it out, due to corruption in
the kernel, filesystem, storage system, etc.  As we only check them when
we read in a page and calculate them when we go to write the page out,
they aren't helpful for shared_buffers corruption, generally speaking.

It might be interesting to consider checking them in 'clean' pages in
shared_buffers in a background process, as that, presumably, *would*
detect shared buffers corruption.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Checksums by default?

2017-01-23 Thread Michael Paquier
On Tue, Jan 24, 2017 at 10:26 AM, Stephen Frost  wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> >> I don't recall ever seeing a checksum failure on a Heroku Postgres
>> >> database,
>
> Not sure how this part of that sentence was missed:
>
> -
> ... even though they were enabled as soon as the feature became
> available.
> -
>
> Which would seem to me to say "the code's been running for a long time
> on a *lot* of systems without throwing a false positive or surfacing a
> bug."

I am reading that similarly to what Tom is seeing: enabling it has
proved Heroku that it did not catch problems in years, meaning that
the performance cost induced by enabling it has paid nothing in
practive, except the insurance to catch up problems should they
happen.

> Given your up-thread concerns that enabling checksums could lead to
> false positives and might surface bugs, that's pretty good indication
> that those concerns are unfounded.

FWIW, I have seen failures that could have been as hardware failures
or where enabling checksums may have helped, but they were on pre-9.3
instances. Now there is a performance penalty in enabling them, but it
solely depends on the page eviction from shared buffers, which is
pretty high for some load patterns I work with, still even with that
we saw a 1~2% output penalty in measurements. Perhaps not everybody
would like to pay this price. FWIW, we are thinking about paying it as
VMs are more sensitive to vmdk-like class of bugs. I am not sure that
everybody would like to pay that. By seeing this thread -hackers
likely are fine with the cost induced.
-- 
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] Checksums by default?

2017-01-23 Thread Peter Geoghegan
On Mon, Jan 23, 2017 at 5:26 PM, Stephen Frost  wrote:
> Not sure how this part of that sentence was missed:
>
> -
> ... even though they were enabled as soon as the feature became
> available.
> -
>
> Which would seem to me to say "the code's been running for a long time
> on a *lot* of systems without throwing a false positive or surfacing a
> bug."

I think you've both understood what I said correctly. Note that I
remain neutral on the question of whether or not checksums should be
enabled by default.

Perhaps I've missed the point entirely, but, I have to ask: How could
there ever be false positives? With checksums, false positives are
simply not allowed. Therefore, there cannot be a false positive,
unless we define checksums as a mechanism that should only find
problems that originate somewhere at or below the filesystem. We
clearly have not done that, so ISTM that checksums could legitimately
find bugs in the checksum code. I am not being facetious.

-- 
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] Online enabling of page level checksums

2017-01-23 Thread Tom Lane
Jim Nasby  writes:
> For a first pass, I think it's acceptable for autovac and vac to notice 
> if a relation needs checksums computed and ignore the VM in that case 
> (make sure it's ignoring all frozen bits too).

> Likewise, for right now I think it's OK to force users that are enabling 
> this to manually connect to datallowcon=false and run vacuum.

I think it's a *complete* mistake to overload vacuum with this
responsibility.  Build a separate tool with a separate scheduling policy.

As one reason why not: vacuum doesn't generally attempt to scan indexes
sequentially at all.  Some of the index AMs might happen to do that, but
touching every page of an index is nowhere in vacuum's charter.  Nor is
there a good reason for index AMs to be involved in the job of placing
checksums, but they'd all have to know about this if you insist on going
through vacuum.

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] Checksums by default?

2017-01-23 Thread Jim Nasby

On 1/23/17 7:15 PM, Tom Lane wrote:

Uhm, Peter G just said that Heroku enables this on all their databases
and have yet to see a false-positive report or an issue with having it
enabled.
That, plus the reports and evidence we've seen in the past couple days,
look like a pretty ringing endorsement for having them.

You must have read a different Peter G than I did.  What I read was


I don't recall ever seeing a checksum failure on a Heroku Postgres
database,

which did not sound like an endorsement to me.


Well, it is pretty good evidence that there's no bugs and that false 
positives aren't a problem. As I mentioned earlier, my bet is that any 
significantly large cloud provider has a ton of things going on behind 
the scenes to prevent oddball (as in non-repeating) errors. When you've 
got 1M+ servers even small probability bugs can become really big problems.


In any case, how can we go about collecting data that checksums help? We 
certainly know people suffer data corruption. We can only guess at how 
many of those incidents would be caught by checksums. I don't see how we 
can get data on that unless we get a lot more users running checksums.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Checksums by default?

2017-01-23 Thread Stephen Frost
Jim,

* Jim Nasby (jim.na...@bluetreble.com) wrote:
> On 1/23/17 6:55 PM, Stephen Frost wrote:
> >* Jim Nasby (jim.na...@bluetreble.com) wrote:
> >>As others have mentioned, right now practically no one enables this,
> >>so we've got zero data on how useful it might actually be.
> >Uhm, Peter G just said that Heroku enables this on all their databases
> >and have yet to see a false-positive report or an issue with having it
> >enabled.
> >
> >That, plus the reports and evidence we've seen in the past couple days,
> >look like a pretty ringing endorsement for having them.
> >
> >I'll ping the RDS crowd and see if they'll tell me what they're doing
> >and what their thoughts are on it.
> 
> Oh, I read the thread as "there's no data to support checksums are
> useful", 

There's been multiple reports on this thread that corruption does
happen.  Sure, it'd be nice if we had a report of it happening with
checksums enabled and where checksums caught it, but I don't see any
basis for an argument that they wouldn't ever catch real-world
bit-flipping corruption.

> IIRC Grant's mentioned in one of his presentations that they enable
> checksums, but getting more explicit info would be good.

Frankly, my recollection is that they wouldn't use PG until it had
page-level checksums, and that they run it on all of their instances,
but I'd like to get confirmation of that, if I can, and also hear if
they've got examples of the checksums we have catching real issues.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Checksums by default?

2017-01-23 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Jim Nasby (jim.na...@bluetreble.com) wrote:
> >> As others have mentioned, right now practically no one enables this,
> >> so we've got zero data on how useful it might actually be.
> 
> > Uhm, Peter G just said that Heroku enables this on all their databases
> > and have yet to see a false-positive report or an issue with having it
> > enabled.
> 
> > That, plus the reports and evidence we've seen in the past couple days,
> > look like a pretty ringing endorsement for having them.
> 
> You must have read a different Peter G than I did.  What I read was
> 
> >> I don't recall ever seeing a checksum failure on a Heroku Postgres
> >> database,

Not sure how this part of that sentence was missed:

-
... even though they were enabled as soon as the feature became 
 
available. 
-

Which would seem to me to say "the code's been running for a long time
on a *lot* of systems without throwing a false positive or surfacing a
bug."

Given your up-thread concerns that enabling checksums could lead to
false positives and might surface bugs, that's pretty good indication
that those concerns are unfounded.

In addition, it shows that big hosting providers were anxious to get the
feature and enabled it immediately for their users, while we debate if
it might be useful for *our* users.  I certainly don't believe that
Heroku or Amazon have all the right answers for everything, but I do
think we should consider that they enabled checksums immediately, along
with the other consultants on this thread who have said the same.

Lastly, I've already pointed out that there were 2 cases recently
reported on IRC of corruption on reasonably modern gear, with a third
comment following that up from Merlin.  These notions that corruption
doesn't happen today, or that we would have heard about it if it had,
also look unfounded from my perspective.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] [COMMITTERS] pgsql: Add function to import operating system collations

2017-01-23 Thread Michael Paquier
On Tue, Jan 24, 2017 at 3:47 AM, Peter Eisentraut
 wrote:
> On 1/22/17 3:47 AM, Michael Paquier wrote:
>> It would be nice at least to avoid an error, still even if we decide
>> to keep it an error I can add a couple of locales in hamster and
>> dangomushi and we are good to go in the buildfarm. Regarding the
>> warning, I have found it useful a couple of times on ArchLinux where
>> no locales are enabled by default.
>
> OK, I just changed it back to a warning.

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] Checksums by default?

2017-01-23 Thread Jim Nasby

On 1/23/17 6:55 PM, Stephen Frost wrote:

* Jim Nasby (jim.na...@bluetreble.com) wrote:

As others have mentioned, right now practically no one enables this,
so we've got zero data on how useful it might actually be.

Uhm, Peter G just said that Heroku enables this on all their databases
and have yet to see a false-positive report or an issue with having it
enabled.

That, plus the reports and evidence we've seen in the past couple days,
look like a pretty ringing endorsement for having them.

I'll ping the RDS crowd and see if they'll tell me what they're doing
and what their thoughts are on it.


Oh, I read the thread as "there's no data to support checksums are 
useful", not "there's no data to support there's little risk of bugs or 
false-positives". I certainly agree that Heroku is a good test of both 
of those.


IIRC Grant's mentioned in one of his presentations that they enable 
checksums, but getting more explicit info would be good.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Checksums by default?

2017-01-23 Thread Tom Lane
Stephen Frost  writes:
> * Jim Nasby (jim.na...@bluetreble.com) wrote:
>> As others have mentioned, right now practically no one enables this,
>> so we've got zero data on how useful it might actually be.

> Uhm, Peter G just said that Heroku enables this on all their databases
> and have yet to see a false-positive report or an issue with having it
> enabled.

> That, plus the reports and evidence we've seen in the past couple days,
> look like a pretty ringing endorsement for having them.

You must have read a different Peter G than I did.  What I read was

>> I don't recall ever seeing a checksum failure on a Heroku Postgres
>> database,

which did not sound like an endorsement to me.

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] Online enabling of page level checksums

2017-01-23 Thread Jim Nasby

On 1/23/17 1:11 PM, David Christensen wrote:

I’m not sure that this will work as-is, unless we’re forcing VACUUM to ignore 
the visibility map.  I had originally considered having this sit on top of 
VACUUm though, we just need a way to iterate over all relations and read every 
page.

Another issue with this (that I think would still exist with the bgworker 
approach) is how to handle databases with datallowconn = 0.  `vacuumdb`, at 
least, explicitly filters out these rows when iterating over databases to 
connect to, so while we could enable them for all databases, we can’t enable 
for the cluster without verifying that these disallowed dbs are already 
checksummed.


For a first pass, I think it's acceptable for autovac and vac to notice 
if a relation needs checksums computed and ignore the VM in that case 
(make sure it's ignoring all frozen bits too).


Likewise, for right now I think it's OK to force users that are enabling 
this to manually connect to datallowcon=false and run vacuum.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Checksums by default?

2017-01-23 Thread Stephen Frost
Jim,

* Jim Nasby (jim.na...@bluetreble.com) wrote:
> As others have mentioned, right now practically no one enables this,
> so we've got zero data on how useful it might actually be.

Uhm, Peter G just said that Heroku enables this on all their databases
and have yet to see a false-positive report or an issue with having it
enabled.

That, plus the reports and evidence we've seen in the past couple days,
look like a pretty ringing endorsement for having them.

I'll ping the RDS crowd and see if they'll tell me what they're doing
and what their thoughts are on it.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Fix a comment in feelist.c

2017-01-23 Thread Tatsuo Ishii
>> I've found a mistake in a comment of StrategyNotifyBgWriter
>> in freelist.c. bgwriterLatch was replaced by bgwprocno in
>> the following commit, but this is remained in the comment.
>> 
>> commit d72731a70450b5e7084991b9caa15cb58a2820df
>> Author: Andres Freund 
>> Date:   Thu Dec 25 18:24:20 2014 +0100
>> 
>> Lockless StrategyGetBuffer clock sweep hot path.
> 
> Looks good to me. I will commit/push the patch if there's no
> objection.

Committed/pushed to all supported branches.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] Generic type subscription

2017-01-23 Thread Jim Nasby

On 1/23/17 1:36 PM, Tom Lane wrote:

Is there a way for a function
in an extension to find the OID of one of its sibling functions?


Obviously there's regprocedure (or it's C equivalent), but then you're 
stuck re-computing at runtime. I've messed around with that a bit in an 
effort to have an extension depend on another extension that allows the 
user to specify it's schema. If you're already doing metaprogramming 
it's not an enormous problem... if you're not already doing that it 
sucks. Trying to make that work in C would be somewhere between 
impossible and a nightmare.


Since this kind of thing affects extensions that depend on extensions, 
it'd certainly be nice if there was some way to address it.


BTW, I actually do use SPI to call one of the reg casts in my variant 
type, but that's just a hack I used in the beginning and haven't gotten 
around to replacing. Since there's a static variable that gets set to 
the relevant OID it's not that bad performance-wise from what I can 
tell, but I suspect that's not something we want to be recommending to 
others...

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Checksums by default?

2017-01-23 Thread Jim Nasby

On 1/23/17 6:14 PM, Peter Geoghegan wrote:

In practice, Postgres checksums do *not* seem to
catch problems. That's been my experience, at least.


For someone running on a bunch of AWS hardware that doesn't really 
surprise me. Presumably, anyone operating at that scale would be quickly 
overwhelmed if odd hardware errors were even remotely common. (Note that 
odd errors aren't the same as an outright failure.)


Where I'd expect this to help is with anyone running a moderate-sized 
data center that doesn't have the kind of monitoring resources a cloud 
provider does.


As for collecting data, I don't really know what more data we can get. 
We get data corruption reports on a fairly regular basis. I think it's a 
very safe bet that CRCs would identify somewhere between 20% and 80%. 
Maybe that number could be better refined, but that's still going to be 
guesswork.


As others have mentioned, right now practically no one enables this, so 
we've got zero data on how useful it might actually be. If the patch to 
make this a GUC goes through then at least we could tell people that 
have experienced corruption to enable this. That might provide some 
data, though the horse is already well out of the barn by then.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Checksums by default?

2017-01-23 Thread Jim Nasby

On 1/23/17 1:30 AM, Amit Kapila wrote:

On Sun, Jan 22, 2017 at 3:43 PM, Tomas Vondra
 wrote:


That being said, I'm ready to do some benchmarking on this, so that we have
at least some numbers to argue about. Can we agree on a set of workloads
that we want to benchmark in the first round?



I think if we can get data for pgbench read-write workload when data
doesn't fit in shared buffers but fit in RAM, that can give us some
indication.  We can try by varying the ratio of shared buffers w.r.t
data.  This should exercise the checksum code both when buffers are
evicted and at next read.  I think it also makes sense to check the
WAL data size for each of those runs.


I tried testing this (and thought I sent an email about it but don't see 
it now :/). Unfortunately, on my laptop I wasn't getting terribly 
consistent runs; I was seeing +/- ~8% TPS. Sometimes checksumps appeared 
to add ~10% overhead, but it was hard to tell.


If someone has a more stable (is in, dedicated) setup, testing would be 
useful.


BTW, I ran the test with small (default 128MB) shared_buffers, scale 50 
(800MB database), sync_commit = off, checkpoint_timeout = 1min, to try 
and significantly increase the rate of buffers being written out.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] IndexBuild Function call fcinfo cannot access memory

2017-01-23 Thread Jia Yu
Dear Tom,

BTW, my current implementation is on PG 9.5...

Thanks,
Jia Yu



Jia Yu, Ph.D. Student

Computer Science

Arizona State University 

Reach me via Jia Yu's Homepage  | GitHub
Repositories 



On Mon, Jan 23, 2017 at 5:12 PM, Jia Yu  wrote:

> Dear Tom,
>
> Thanks for your reply!
>
> Actually, I already compiled the source code with "-O0" and then got those
> backtraces.
>
> The hippobuild function just starts building an index access method and it
> will call hippobuildcallback to iterate each parent table tuple in its
> function body.
>
> What makes me fell very confused is that the same code works when I call
> "make check" to run regression test (testing my index) but fails at running
> normal PG server.
>
> Here are the lines before and after the problematic line hippo.c 154. It
> is the first line of this function and did nothing.
>
> On the other hand, I will try to adjust my code to fit in PG 9.6 as you
> suggested.
>
> --
> static void hippobuildcallback()
> {
> .
> .
> .
> *Line 148* }
> *Line 149* /*
> *Line 150* *This function initializes the entire Hippo. It will call
> buildcallback many times.
> *Line 151* */
> *Line 152* Datum
> *Line 153* hippobuild(PG_FUNCTION_ARGS)
> *Line 154* {
> *Line 155* /*
>  * This is some routine declarations for variables
>*/
> Relation heap = (Relation) PG_GETARG_POINTER(0);
> Relation index = (Relation) PG_GETARG_POINTER(1);
> IndexInfo  *indexInfo = (IndexInfo *) PG_GETARG_POINTER(2);
> IndexBuildResult *result;
> double reltuples;
> HippoBuildState buildstate;
> Buffer buffer;
> ..
> --
>
> Any hints for this will be greatly appreciated!
>
>
> Thank you very much!
>
> Jia Yu
>
> On Mon, Jan 23, 2017 at 2:00 PM, Tom Lane  wrote:
>
>> Jia Yu  writes:
>> > However, these methods don't work in the normal PG server. It gave me
>> > "segmentation fault"
>> > ...
>> > Here is my backtrace. It looks like I cannot access fcinfo. Can you
>> help me
>> > about this? Or just some hints? I have been struggling with this problem
>> > for weeks.
>>
>> What's the problematic line (hippo.c:154) doing?  What's before
>> that in the hippobuild function?
>>
>> I wouldn't put much stock in the "Cannot access memory" message ... that
>> just means gdb is confused, which it frequently is.  But possibly
>> building with -O0 would make it less confused.
>>
>> BTW, I gather from the reference to OidFunctionCall3Coll that you're
>> using PG 9.5 or older.  It'd be a good idea to switch to 9.6, which has
>> noticeably cleaner indexam APIs that allow some compiler error checking
>> on those calls.
>>
>> regards, tom lane
>>
>
>


Re: [HACKERS] Checksums by default?

2017-01-23 Thread Merlin Moncure
On Sat, Jan 21, 2017 at 12:35 PM, Tom Lane  wrote:
> Andres Freund  writes:
>> Sure, it might be easy, but we don't have it.  Personally I think
>> checksums just aren't even ready for prime time. If we had:
>> - ability to switch on/off at runtime (early patches for that have IIRC
>>   been posted)
>> - *builtin* tooling to check checksums for everything
>> - *builtin* tooling to compute checksums after changing setting
>> - configurable background sweeps for checksums
>
> Yeah, and there's a bunch of usability tooling that we don't have,
> centered around "what do you do after you get a checksum error?".
> AFAIK there's no way to check or clear such an error; but without
> such tools, I'm afraid that checksums are as much of a foot-gun
> as a benefit.

I see your point here, but they sure saved my ass with that pl/sh
issue.  So I'm inclined to lightly disagree; there are good arguments
either way.

merlin


-- 
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] Checksums by default?

2017-01-23 Thread Peter Geoghegan
On Sat, Jan 21, 2017 at 9:09 AM, Tom Lane  wrote:
> Not at all; I just think that it's not clear that they are a net win
> for the average user, and so I'm unconvinced that turning them on by
> default is a good idea.  I could be convinced otherwise by suitable
> evidence.  What I'm objecting to is turning them on without making
> any effort to collect such evidence.

+1

One insight Jim Gray has in the classic paper "Why Do Computers Stop
and What Can Be Done About It?" [1] is that fault-tolerant hardware is
table stakes, and so most failures are related to operator error, and
to a lesser extent software bugs. The paper is about 30 years old.

I don't recall ever seeing a checksum failure on a Heroku Postgres
database, even though they were enabled as soon as the feature became
available. I have seen a few corruption problems brought to light by
amcheck, though, all of which were due to bugs in software.
Apparently, before I joined Heroku there were real reliability
problems with the storage subsystem that Heroku Postgres runs on (it's
a pluggable storage service from a popular cloud provider -- the
"pluggable" functionality would have made it fairly novel at the
time). These problems were something that the Heroku Postgres team
dealt with about 6 years ago. However, anecdotal evidence suggests
that the reliability of the same storage system *vastly* improved
roughly a year or two later. We still occasionally lose drives, but
drives seem to fail fast in a fashion that lets us recover without
data loss easily. In practice, Postgres checksums do *not* seem to
catch problems. That's been my experience, at least.

Obviously every additional check helps, and it may be something we can
do without any appreciable downside. I'd like to see a benchmark.

[1] http://www.hpl.hp.com/techreports/tandem/TR-85.7.pdf
-- 
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] GSoC 2017

2017-01-23 Thread Jim Nasby

On 1/23/17 3:45 PM, Peter van Hardenberg wrote:

A new currency type would be nice, and if kept small in scope, might be
manageable.


I'd be rather nervous about this. My impression of community consensus 
on this is a currency type that doesn't somehow support conversion 
between different currencies is pretty useless, and supporting 
conversions opens a 55 gallon drum of worms. I could certainly be 
mistaken in my impression, but I think there'd need to be some kind of 
consensus on what a currency type should do before putting that up for GSoC.


But, speaking of types, I wish we had a timestamp type that stored what 
the original timezone was, as well as the relevant TZDATA entry that was 
in place for that timestamp when it was created. Since it'd be 
completely impractical to store TZDATA as part of the dataum, there 
would need to be an immutable catalog table that stored the contents of 
TZDATA any time it changed, as well as a fast way to find the surrogate 
key for the current TZDATA.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] IndexBuild Function call fcinfo cannot access memory

2017-01-23 Thread Jia Yu
Dear Tom,

Thanks for your reply!

Actually, I already compiled the source code with "-O0" and then got those
backtraces.

The hippobuild function just starts building an index access method and it
will call hippobuildcallback to iterate each parent table tuple in its
function body.

What makes me fell very confused is that the same code works when I call
"make check" to run regression test (testing my index) but fails at running
normal PG server.

Here are the lines before and after the problematic line hippo.c 154. It is
the first line of this function and did nothing.

On the other hand, I will try to adjust my code to fit in PG 9.6 as you
suggested.

--
static void hippobuildcallback()
{
.
.
.
*Line 148* }
*Line 149* /*
*Line 150* *This function initializes the entire Hippo. It will call
buildcallback many times.
*Line 151* */
*Line 152* Datum
*Line 153* hippobuild(PG_FUNCTION_ARGS)
*Line 154* {
*Line 155* /*
 * This is some routine declarations for variables
   */
Relation heap = (Relation) PG_GETARG_POINTER(0);
Relation index = (Relation) PG_GETARG_POINTER(1);
IndexInfo  *indexInfo = (IndexInfo *) PG_GETARG_POINTER(2);
IndexBuildResult *result;
double reltuples;
HippoBuildState buildstate;
Buffer buffer;
..
--

Any hints for this will be greatly appreciated!


Thank you very much!

Jia Yu

On Mon, Jan 23, 2017 at 2:00 PM, Tom Lane  wrote:

> Jia Yu  writes:
> > However, these methods don't work in the normal PG server. It gave me
> > "segmentation fault"
> > ...
> > Here is my backtrace. It looks like I cannot access fcinfo. Can you help
> me
> > about this? Or just some hints? I have been struggling with this problem
> > for weeks.
>
> What's the problematic line (hippo.c:154) doing?  What's before
> that in the hippobuild function?
>
> I wouldn't put much stock in the "Cannot access memory" message ... that
> just means gdb is confused, which it frequently is.  But possibly
> building with -O0 would make it less confused.
>
> BTW, I gather from the reference to OidFunctionCall3Coll that you're
> using PG 9.5 or older.  It'd be a good idea to switch to 9.6, which has
> noticeably cleaner indexam APIs that allow some compiler error checking
> on those calls.
>
> regards, tom lane
>


Re: [HACKERS] Contrib: alternative MATERIALIZED VIEWs

2017-01-23 Thread Jim Nasby

On 1/23/17 5:38 PM, Nico Williams wrote:

Attached is an alternative implementation of MATERIALIZED VIEWs.


Interesting. I don't see this being accepted into core because it's 
plpgsql and it depends on the user to track what criteria to use to 
apply the update. The second item is the biggest issue.


That said, I think this would be useful to some people as an extension. 
I suggest you put it on github (or equivalent) and upload it to 
http://pgxn.org.


In terms of community support, the next step is to get statement-level 
support for NEW and OLD, something I think Kevin has been working on.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Contrib: pqasyncnotifier.c -- a shell command client for LISTEN

2017-01-23 Thread Marko Tiikkaja
On Tue, Jan 24, 2017 at 12:40 AM, Nico Williams 
wrote:

> psql(1) does not output notifications asynchronously, as it does not
> check for them when idle.  This makes it difficult to script handling of
> NOTIFYs.
>
> Attached is pqasyncnotifier.c, a simple command that allows one to
> handle NOTIFYs asynchronously.
>

Did you forget the attachment?


.m


[HACKERS] Contrib: pqasyncnotifier.c -- a shell command client for LISTEN

2017-01-23 Thread Nico Williams
psql(1) does not output notifications asynchronously, as it does not
check for them when idle.  This makes it difficult to script handling of
NOTIFYs.

Attached is pqasyncnotifier.c, a simple command that allows one to
handle NOTIFYs asynchronously.

Cheers,

Nico
-- 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Contrib: alternative MATERIALIZED VIEWs

2017-01-23 Thread Nico Williams
Attached is an alternative implementation of MATERIALIZED VIEWs.

The idea is to explore possible enahncements to the PostgreSQL
MATERIALIZED VIEW features.

Features:

 - All SQL-coded.

 - Keeps history of deltas computed at each refresh.

 - Allows DMLs of the materialized view, recording the changes in the
   same way as deltas from refreshes.

   This allows one to code TRIGGERs which update materialized views
   directly.

   Where synchronous updates of an MV can be fast, a TRIGGER can do it
   by querying the source view with additional constraints derived from
   the OLD/NEW rows, and then apply DMLs to the materialization table.

 - MVs can be marked as needing a refresh.

   This is useful where a synchronous update would be too slow.  Use a
   TRIGGER to mark the MV as needing a refresh and NOTIFY a waiting
   service.

 - Refreshes have the same concurrency semantics as REFRESH MATERIALIZED
   VIEW CONCURRENTLY.

 - Allows indices and constraints to be added to the materialzation
   table.

Issues:

 - NULLs in columns of the VIEW cause spurious deltas to be recorded for
   every refresh.  This is because two rows which are equal when
   considering NULLs to be equal are... not equal as far as SQL goes,
   thus such rows always appear to be deleted and inserted.

   This implementation uses a NATURAL FULL OUTER JOIN to compute the
   deltas between a before and an after materialization of a view during
   a refresh.  This avoids having to generate a USING(), which is a very
   convenient simplification, but also part of the source of this
   problem with NULLs.  (The history table has two record-type columns
   to hold entire rows.)

 - No integration.

 - Wonky column names in the history table ("awld", "noo").

Ideas:

 - CREATE MATERIALIZED VIEW should have these additional options, and
   ALTER MATERIALIZED VIEW should allow these to be specified after
   creation:

- WITH [UNLOGGED] HISTORY TABLE schema_name.table_name

- WITH PRIMARY KEY (column_list) -- probably not in ALTER MV though

  A PK on an MV does make sense when one considers the admonition in
  the PG docs to not have duplicate rows in the view...  Besides, an
  MV has a materialization table, and tables generally should have
  PKs!

- WITH CONSTRAINT  (same as in ALTER TABLE ADD
  constraint)


Also, a new type of JOIN might be useful: one that joins using only
columns that are part of the PK of both table sources.  Obviously this
would not be a generally-applicable JOIN type, as it would not work for
table sources that are subqueries or plain views...  But it would be
useful here for generating the FULL OUTER JOIN needed for computing
deltas between tables of the same form.

Nico
-- 


pseudo_mat_views.sql
Description: application/sql

-- 
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 : For Auto-Prewarm.

2017-01-23 Thread Jim Nasby
I took a look at this again, and it doesn't appear to be working for me. The 
library is being loaded during startup, but I don't see any further activity in 
the log, and I don't see an autoprewarm file in $PGDATA.

There needs to be some kind of documentation change as part of this patch.

I'm not sure the default GUC setting of 0 makes sense. If you've loaded the 
module, presumably you want it to be running. I think it'd be nice if the GUC 
had a -1 setting that meant to use checkpoint_timeout.

Having the GUC be restart-only is also pretty onerous. I don't think it'd be 
hard to make the worker respond to a reload... there's code in the autovacuum 
launcher you could use as an example.

I'm also wondering if this really needs to be a permanently running process... 
perhaps the worker could simply be started as necessary? Though maybe that 
means it wouldn't run at shutdown. Also not sure if it could be relaunched when 
a reload happens.

I'm marking this as waiting on author for now, because it's not working for me 
and needs documentation.

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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

2017-01-23 Thread Corey Huinker
On Mon, Jan 23, 2017 at 4:12 PM, Corey Huinker 
wrote:

> I do not like the choice of "elseif", which exists in PHP & VB. PL/pgSQL
>> has "ELSIF" like perl, that I do not like much, though. Given the syntax
>> and behavioral proximity with cpp, I suggest that "elif" would be a better
>> choice.
>>
>
> I'm personally indifferent to elif vs elsif vs elseif, but I think elseif
> was the consensus. It's easy enough to change.
>

Went with elsif to follow pl/pgsql. In reviewing the original thread it
seemed that "elif" was linked to "fi" and that got some negative feedback.


>
>
>>
>> Documentation: I do not think that the systematic test-like example in
>> the documentation is relevant, a better example should be shown. The list
>> of what is considered true or false should be told explicitely, not end
>> with "etc" that is everyone to guess.
>>
>
> It was copied from the regression test. I knew there would be follow-up
> suggestions for what should be shown.
>

Work on this is pending discussion of what true/false values we should
allow, and if values outside of those is an error.


>>
>>   case ...: case ...:
>>
>> The project rules are to add explicit /* PASSTHROUGH */ comment.
>>
>
> Will do.
>

I went looking for other examples of explicit /* PASSTHROUGH */ comments
and could find none. Could you explain what it is you want me to fix with
the switch statements?


Re: [HACKERS] Protect syscache from bloating with negative cache entries

2017-01-23 Thread Jim Nasby

On 1/21/17 6:42 PM, Jim Nasby wrote:

On 12/26/16 2:31 AM, Kyotaro HORIGUCHI wrote:

The points of discussion are the following, I think.

1. The first patch seems working well. It costs the time to scan
   the whole of a catcache that have negative entries for other
   reloids. However, such negative entries are created by rather
   unusual usages. Accesing to undefined columns, and accessing
   columns on which no statistics have created. The
   whole-catcache scan occurs on ATTNAME, ATTNUM and
   STATRELATTINH for every invalidation of a relcache entry.


I took a look at this. It looks sane, though I've got a few minor
comment tweaks:

+ *Remove negative cache tuples maching a partial key.
s/maching/matching/

+/* searching with a paritial key needs scanning the whole cache */

s/needs/means/

+ * a negative cache entry cannot be referenced so we can remove

s/referenced/referenced,/

I was wondering if there's a way to test the performance impact of
deleting negative entries.


I did a make installcheck run with CATCACHE_STATS to see how often we 
get negative entries in the 3 caches affected by this patch. The caches 
on pg_attribute get almost no negative entries. pg_statistic gets a good 
amount of negative entries, presumably because we start off with no 
entries in there. On a stable system that presumably won't be an issue, 
but if temporary tables are in use and being analyzed I'd think there 
could be a moderate amount of inval traffic on that cache. I'll leave it 
to a committer to decide if they thing that's an issue, but you might 
want to try and quantify how big a hit that is. I think it'd also be 
useful to know how much bloat you were seeing in the field.


The patch is currently conflicting against master though, due to some 
caches being added. Can you rebase? BTW, if you set a slightly larger 
context size on the patch you might be able to avoid rebases; right now 
the patch doesn't include enough context to uniquely identify the chunks 
against cacheinfo[].

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] src/test/README for logical replication

2017-01-23 Thread Craig Ringer
src/test/README wasn't updated for the new src/test/subscription entry.

Patch attached.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 491587b3fcb7a2fa2689171161354a708efa3cc1 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Tue, 24 Jan 2017 06:15:37 +0800
Subject: [PATCH] Mention logical replication tests in src/test/README

---
 src/test/README | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/test/README b/src/test/README
index 62395e7..0019782 100644
--- a/src/test/README
+++ b/src/test/README
@@ -37,5 +37,8 @@ regress/
 ssl/
   Tests to exercise and verify SSL certificate handling
 
+subscription/
+  Tests for logical replication
+
 thread/
   A thread-safety-testing utility used by configure
-- 
2.5.5


-- 
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 proposal

2017-01-23 Thread David Steele
Hi Venkata,

On 11/8/16 5:47 PM, Venkata B Nagothi wrote:
> Attached is the 2nd version of the patch with some enhancements.

Here's my review of the patch.

1) There are a number of whitespace error when applying the patch:

$ git apply ../other/recoveryTargetIncomplete.patch-version2
../other/recoveryTargetIncomplete.patch-version2:158: indent with spaces.
else
../other/recoveryTargetIncomplete.patch-version2:160: space before tab
in indent.
ereport(LOG,
../other/recoveryTargetIncomplete.patch-version2:166: indent with spaces.
  /*
../other/recoveryTargetIncomplete.patch-version2:167: indent with spaces.
   * This is the position where we can
choose to shutdown, pause
../other/recoveryTargetIncomplete.patch-version2:168: indent with spaces.
   * or promote at the end-of-the-wal if the
intended recovery
warning: squelched 10 whitespace errors
warning: 15 lines add whitespace errors.

The build did succeed after applying the patch.

2) There is no documentation.  Serious changes to recovery are going to
need to be documented very carefully.  It will also help during review
to determine you intent.

3) There are no tests.  I believe that
src/test/recovery/t/006_logical_decoding.pl would be the best place to
insert your tests.

4) I'm not convinced that fatal errors by default are the way to go.
It's a pretty big departure from what we have now.

Also, I don't think it's very intuitive how recovery_target_incomplete
works.  For instance, if I set recovery_target_incomplete=promote and
set recovery_target_time, but pick a backup after the specified time,
then there will be an error "recovery_target_time %s is older..." rather
than a promotion.

It seems to me that there needs to be a separate setting to raise a
fatal error.

5) There are really two patches here.  One deals with notifying the user
that replay to their recovery target is not possible (implemented here
with fatal errors) and the other allows options when their recovery
target appears to be possible but never arrives.

As far as I can see, at this point, nobody has really signed on to this
approach.  I believe you will need a consensus before you can proceed
with a patch this disruptive.

A better approach may be to pull the first part out and raise warnings
instead.  This will allow you to write tests and make sure that your
logic is correct without major behavioral changes to Postgres.

I'm marking this as "Waiting on Author", but honestly I think "Returned
with Feedback" might be more appropriate.  I'll leave that up to the CFM.

-- 
-David
da...@pgmasters.net


-- 
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] GSoC 2017

2017-01-23 Thread Peter van Hardenberg
A new currency type would be nice, and if kept small in scope, might be
manageable. Bringing Christoph Berg's PostgreSQL-units into core and
extending it could be interesting. Peter E's URL and email types might be
good candidates. What else? Informix Datablades had a media type way back
in the day... That's still a gap in community Postgres.

On Mon, Jan 16, 2017 at 6:43 PM, Jim Nasby  wrote:

> On 1/13/17 3:09 PM, Peter van Hardenberg wrote:
>
>> A new data type, and/or a new index type could both be nicely scoped
>> bits of work.
>>
>
> Did you have any particular data/index types in mind?
>
> Personally I'd love something that worked like a python dictionary, but
> I'm not sure how that'd work without essentially supporting a variant data
> type. I've got code for a variant type[1], and I don't think there's any
> holes in it, but the casting semantics are rather ugly. IIRC that problem
> appeared to be solvable if there was a hook in the current casting code
> right before Postgres threw in the towel and said a cast was impossible.
>
> 1: https://github.com/BlueTreble/variant/
>
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)
>



-- 
Peter van Hardenberg
San Francisco, California
"Everything was beautiful, and nothing hurt."—Kurt Vonnegut


Re: [HACKERS] PoC: Grouped base relation

2017-01-23 Thread David Rowley
On 20 January 2017 at 00:22, Antonin Houska  wrote:
> Sorry, it was my thinko - I somehow confused David's CROSS JOIN example with
> this one. If one side of the join clause is unique and the other becomes
> unique due to aggregation (and if parallel processing is not engaged) then
> neither combinefn nor multiplyfn should be necessary before the finalfn.

Yes, if the join can be detected not to duplicate the groups then a
normal aggregate node can be pushed below the join. No need for
Partial Aggregate, or Finalize Aggregate nodes.

I've a pending patch in the commitfest named "Unique Joins", which
aims teach the planner about the unique properties of joins. So you
should just have both stages of aggregation occur for now, and that
can be improved on once the planner is a bit smart and knows about
unique joins.


-- 
 David Rowley   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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

2017-01-23 Thread Corey Huinker
>
> I do not like the choice of "elseif", which exists in PHP & VB. PL/pgSQL
> has "ELSIF" like perl, that I do not like much, though. Given the syntax
> and behavioral proximity with cpp, I suggest that "elif" would be a better
> choice.
>

I'm personally indifferent to elif vs elsif vs elseif, but I think elseif
was the consensus. It's easy enough to change.


>
> Documentation: I do not think that the systematic test-like example in the
> documentation is relevant, a better example should be shown. The list of
> what is considered true or false should be told explicitely, not end with
> "etc" that is everyone to guess.
>

It was copied from the regression test. I knew there would be follow-up
suggestions for what should be shown.


>
> Tests: On principle they should include echos in all non executed branches
> to reassure that they are indeed not executed.
>

Will do.


>
> Also, no error cases are tested. They should be. Maybe it is not very easy
> to test some cases which expect to make psql generate errors, but I feel
> they are absolutely necessary for such a feature.
>

Will do.


>
> 1: unrecognized value "whatever" for "\if"; assuming "on"
>
> I do not think that the script should continue with such an assumption.
>

I agree, and this means we can't use ParseVariableBool() as-is. I already
broke out argument reading to it's own function, knowing that it'd be the
stub for expressions. So I guess we start that now. What subset of true-ish
values do you think we should support? If we think that real expressions
are possible soon, we could only allow 'true' and 'false' for now, but if
we expect that expressions might not make it into v10, then perhaps we
should support the same text values that coerce to booleans on the server
side.


>
> 2: encountered \else after \else ... "# ERROR BEFORE"
>
> Even with ON_ERROR_STOP activated the execution continues.


> 3: no \endif
>
> Error reported, but it does not stop even with ON_ERROR_STOP.
>
> 4: include with bad nesting...
>
> Again, even with ON_ERROR_STOP, the execution continues...
>

All valid issues. Will add those to the regression as well (with
ON_ERROR_STOP disabled, obviously).


>
>
> Code:
>
>   -   if (status != PSQL_CMD_ERROR)
>   +   if ((status != PSQL_CMD_ERROR) && psqlscan_branch_active(scan_st
> ate))
>
> Why the added parenthesis?
>

Will fix.


>
>   case ...: case ...:
>
> The project rules are to add explicit /* PASSTHROUGH */ comment.
>

Will do.


>
> There are spaces at the end of one line in a comment about
> psqlscan_branch_end_state.
>
> There are multiple "TODO" comments in the file... Is it done or work in
> progress?
>

I forgot to remove them. But it would be wildly optimistic of me to think
there would be no more work for me after the first patch submission.


>
> Usually in pg comments about a function do not repeat the function name.
> For very short comment, they are /* inlined */ on one line. Use pg comment
> style.


In that case, I was copying the style found in other functions psqlscan.l -
I'm happy to remove it.


Re: [HACKERS] Allowing nonzero return codes from \quit

2017-01-23 Thread Tom Lane
Fabien COELHO  writes:
>> I didn't think about it too much, but I don't see why a user couldn't set
>> one of those error codes.
>> I did, however, think that any attempt to set an exit_code outside of
>> [0,127] would itself be an error, resulting in an exit code of 3.

> Hmmm. Maybe it should let the user shoots its own foot if desired, just a 
> reminder in the doc of a possible interference with existing codes would 
> be enough?

Yeah, I see no reason why scripts shouldn't be allowed to return one of
those codes if they like.  We do need to constrain it to [0,127] for
sanity though, or possibly [0,125] --- doesn't POSIX mandate special
meaning for 126 and 127?

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] IndexBuild Function call fcinfo cannot access memory

2017-01-23 Thread Tom Lane
Jia Yu  writes:
> However, these methods don't work in the normal PG server. It gave me
> "segmentation fault"
> ...
> Here is my backtrace. It looks like I cannot access fcinfo. Can you help me
> about this? Or just some hints? I have been struggling with this problem
> for weeks.

What's the problematic line (hippo.c:154) doing?  What's before
that in the hippobuild function?

I wouldn't put much stock in the "Cannot access memory" message ... that
just means gdb is confused, which it frequently is.  But possibly
building with -O0 would make it less confused.

BTW, I gather from the reference to OidFunctionCall3Coll that you're
using PG 9.5 or older.  It'd be a good idea to switch to 9.6, which has
noticeably cleaner indexam APIs that allow some compiler error checking
on those calls.

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] PoC plpgsql - possibility to force custom or generic plan

2017-01-23 Thread Jim Nasby

On 1/23/17 2:10 PM, Pavel Stehule wrote:

Comments, notes?


+1 on the idea. It'd also be nice if we could expose control of plans 
for dynamic SQL, though I suspect that's not terribly useful without 
some kind of global session storage.


A couple notes on a quick read-through:

Instead of paralleling all the existing namespace stuff, I wonder if 
it'd be better to create explicit block infrastructure. AFAIK PRAGMAs 
are going to have a lot of the same requirements (certainly the nesting 
is the same), and we might want more of this king of stuff in the 
future. (I've certainly wished I could set a GUC in a plpgsql block and 
have it's settings revert when exiting the block...)


Perhaps that's as simple as renaming all the existing _ns_* functions to 
_block_ and then adding support for pragmas...


Since you're adding cursor_options to PLpgSQL_expr it should probably be 
removed as an option to exec_*.


finit_ would be better named free_.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Undefined psql variables

2017-01-23 Thread Fabien COELHO



Back in the day, PG allowed ":" as a generic operator name, making
this even worse; but I think the only remaining SQL syntax that could
include a colon is array slicing.


Ok, so the behavior of replacing ":unknown" by same cannot be changed.

Some fun:

  \set 1 1
  SELECT ('{1,2,3,4,5,6,7,8,9,10,11,12}'::INT[])[1:1];
  -- yields 11

--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Allowing nonzero return codes from \quit

2017-01-23 Thread Fabien COELHO



As \q does not currently have an argument, this seems an easy and
reasonnable extension.

However, currently there are 4 existing exit status for psql: 0 (ok), 1
(fatal error), 2 (connection error), 3 (script error...). +128 status are
also already used when killing a psql process.


I didn't think about it too much, but I don't see why a user couldn't set
one of those error codes.
I did, however, think that any attempt to set an exit_code outside of
[0,127] would itself be an error, resulting in an exit code of 3.


Hmmm. Maybe it should let the user shoots its own foot if desired, just a 
reminder in the doc of a possible interference with existing codes would 
be enough?


--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [COMMITTERS] pgsql: Generate fmgr prototypes automatically

2017-01-23 Thread Tom Lane
Peter Eisentraut  writes:
> On 1/18/17 7:38 AM, Tom Lane wrote:
>> Why do fmgroids.h, fmgrprotos.h, and fmgrtab.c now get mentioned
>> twice?  I suspect there is something broken about the parallelization.

> I've pushed a fix for this.

Seems to fix the problem for me --- thanks!

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] IndexBuild Function call fcinfo cannot access memory

2017-01-23 Thread Jia Yu
Dear hackers,

Currently, I am developing a backend index access method for my research
project.

I built corresponding routines such as MyIndexbuild, MyIndexInsert, and so
on and put them in "src/backend/access/hippo" (hippo is my index's name). I
also added new entries in corresponding catalog files:

src/include/catalog/pg_am.h
src/include/catalog/pg_amop.h
src/include/catalog/pg_amproc.h
src/include/catalog/pg_opclass.h
src/include/catalog/pg_opfamily.h
src/include/catalog/pg_proc.h

Now I can successfully build/query/update my new index by executing PG
source code regression test (make check). Everything looks fine.


However, these methods don't work in the normal PG server. It gave me
"segmentation fault"

I entered this SQL command in psql:
create index hippo_idx on hippo_tbl using hippo(id2);

Then got:
Segmentation fault (core dumped)

Here is my backtrace. It looks like I cannot access fcinfo. Can you help me
about this? Or just some hints? I have been struggling with this problem
for weeks.

Thank you all in advance!


#0  0x004673d2 in hippobuild (
fcinfo=) at hippo.c:154
#1  0x00956e64 in OidFunctionCall3Coll (functionId=5005,
collation=0,
arg1=140162511383536, arg2=140162511390256, arg3=19885792) at
fmgr.c:1649
#2  0x00555809 in index_build (heapRelation=0x7f7a20b3abf0,
indexRelation=0x7f7a20b3c630, indexInfo=0x12f6ee0, isprimary=0 '\000',
isreindex=0 '\000') at index.c:2025
#3  0x00554551 in index_create (heapRelation=0x7f7a20b3abf0,
indexRelationName=0x12f9378 "hippo_idx", indexRelationId=20668,
relFileNode=0, indexInfo=0x12f6ee0, indexColNames=0x12f7370,
accessMethodObjectId=5000, tableSpaceId=0, collationObjectId=0x12f7990,
classObjectId=0x12f79b0, coloptions=0x12f79d0, reloptions=19895952,
isprimary=0 '\000', isconstraint=0 '\000', deferrable=0 '\000',
initdeferred=0 '\000', allow_system_table_mods=0 '\000',
skip_build=0 '\000', concurrent=0 '\000', is_internal=0 '\000',
if_not_exists=0 '\000') at index.c:1100
#4  0x0062727b in DefineIndex (relationId=12476, stmt=0x12f6ff8,
indexRelationId=0, is_alter_table=0 '\000', check_rights=1 '\001',
skip_build=0 '\000', quiet=0 '\000') at indexcmds.c:606
#5  0x008077a6 in ProcessUtilitySlow (parsetree=0x127c740,
queryString=0x127ba48 "create index hippo_idx on hippo_tbl using
hippo(id2) with (density=20);\n", context=PROCESS_UTILITY_TOPLEVEL,
params=0x0,
dest=0xe19a40 , completionTag=0x7fff7819aae0 "")
at utility.c:1260
#6  0x00806c30 in standard_ProcessUtility (parsetree=0x127c740,
queryString=0x127ba48 "create index hippo_idx on hippo_tbl using
hippo(id2) with (density=20);\n", context=PROCESS_UTILITY_TOPLEVEL,
params=0x0,
dest=0xe19a40 , completionTag=0x7fff7819aae0 "")
at utility.c:893
#7  0x00805d81 in ProcessUtility (parsetree=0x127c740,
queryString=0x127ba48 "create index hippo_idx on hippo_tbl using
hippo(id2) with (density=20);\n", context=PROCESS_UTILITY_TOPLEVEL,
params=0x0,
dest=0xe19a40 , completionTag=0x7fff7819aae0 "")
at utility.c:335
#8  0x00804dd3 in PortalRunUtility (portal=0x12f0d88,
utilityStmt=0x127c740, isTopLevel=1 '\001', dest=0xe19a40 ,
completionTag=0x7fff7819aae0 "") at pquery.c:1187
#9  0x00804fa1 in PortalRunMulti (portal=0x12f0d88,
isTopLevel=1 '\001', dest=0xe19a40 ,
altdest=0xe19a40 , completionTag=0x7fff7819aae0 "")
at pquery.c:1318
#10 0x008044e5 in PortalRun (portal=0x12f0d88,
count=9223372036854775807, isTopLevel=1 '\001',
dest=0xe19a40 , altdest=0xe19a40 ,
completionTag=0x7fff7819aae0 "") at pquery.c:816
#11 0x007fe2af in exec_simple_query (
query_string=0x127ba48 "create index hippo_idx on hippo_tbl using
hippo(id2) with (density=20);\n") at postgres.c:1104
#12 0x008025af in PostgresMain (argc=6, argv=0x1244a40,
dbname=0x124c3a0 "postgres", username=0x124c3a0 "postgres")
at postgres.c:4025
#13 0x006d3513 in main (argc=6, argv=0x1244a40) at main.c:219


Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

2017-01-23 Thread Fabien COELHO



A few comments:


Argh, better with the attachements:-(

--
Fabien.

if-error-1.sql
Description: application/sql


if-error-2.sql
Description: application/sql


if-error-3.sql
Description: application/sql


if-error-4.sql
Description: application/sql

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

2017-01-23 Thread Fabien COELHO



And here's the patch. I've changed the subject line and will be submitting
a new entry to the commitfest.


A few comments:

Patch applies, make check is ok, but currently really too partial.

I do not like the choice of "elseif", which exists in PHP & VB. PL/pgSQL 
has "ELSIF" like perl, that I do not like much, though. Given the syntax 
and behavioral proximity with cpp, I suggest that "elif" would be a better 
choice.


Documentation: I do not think that the systematic test-like example in the 
documentation is relevant, a better example should be shown. The list of 
what is considered true or false should be told explicitely, not end with 
"etc" that is everyone to guess.


Tests: On principle they should include echos in all non executed branches 
to reassure that they are indeed not executed.


Also, no error cases are tested. They should be. Maybe it is not very easy 
to test some cases which expect to make psql generate errors, but I feel 
they are absolutely necessary for such a feature.


1: unrecognized value "whatever" for "\if"; assuming "on"

I do not think that the script should continue with such an assumption.

2: encountered \else after \else ... "# ERROR BEFORE"

Even with ON_ERROR_STOP activated the execution continues.

3: no \endif

Error reported, but it does not stop even with ON_ERROR_STOP.

4: include with bad nesting...

Again, even with ON_ERROR_STOP, the execution continues...


Code:

  -   if (status != PSQL_CMD_ERROR)
  +   if ((status != PSQL_CMD_ERROR) && psqlscan_branch_active(scan_state))

Why the added parenthesis?

  case ...: case ...:

The project rules are to add explicit /* PASSTHROUGH */ comment.

There are spaces at the end of one line in a comment about 
psqlscan_branch_end_state.


There are multiple "TODO" comments in the file... Is it done or work in 
progress?


Usually in pg comments about a function do not repeat the function name. 
For very short comment, they are /* inlined */ on one line. Use pg comment 
style.


--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [COMMITTERS] pgsql: Generate fmgr prototypes automatically

2017-01-23 Thread Peter Eisentraut
On 1/18/17 7:38 AM, Tom Lane wrote:
> Why do fmgroids.h, fmgrprotos.h, and fmgrtab.c now get mentioned
> twice?  I suspect there is something broken about the parallelization.
> If indeed multiple instances of gmake are writing these files
> concurrently, that's likely to result in irreproducible build failures.

I've pushed a fix for this.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] PoC plpgsql - possibility to force custom or generic plan

2017-01-23 Thread Pavel Stehule
Hi,

this patch is based on discussions related to plpgsql2 project.

Currently we cannot to control plan cache from plpgsql directly. We can use
dynamic SQL if we can enforce oneshot plan - but it means little bit less
readable code (if we enforce dynamic SQL from performance reasons). It
means so the code cannot be checked by plpgsql check too.

The plan cache subsystem allows some control by options
CURSOR_OPT_GENERIC_PLAN and CURSOR_OPT_CUSTOM_PLAN. So we just a interface
how to use these options from PLpgSQL. I used Ada language feature (used in
PL/SQL too) - PRAGMA statement. It allows to set compiler directives. The
syntax of PRAGMA statements allows to set a level where entered compiler
directive should be applied. It can works on function level or block level.

Attached patch introduces PRAGMA plan_cache with options: DEFAULT,
FORCE_CUSTOM_PLAN, FORCE_GENERIC_PLAN. Plan cache is partially used every
time - the parser/analyzer result is cached every time.

Examples:

CREATE OR REPLACE FUNCTION foo(a int)
RETURNS int AS  $$
DECLARE ..
BEGIN

   DECLARE
 /* block level (local scope) pragma */
 PRAGMA plan_cache(FORCE_CUSTOM_PLAN);
   BEGIN
 SELECT /* slow query - dynamic sql is not necessary */
   END;

 END;

Benefits:

1. remove one case where dynamic sql is necessary now - security, static
check
2. introduce PRAGMAs - possible usage: autonomous transactions, implicit
namespaces settings (namespace for auto variables, namespace for function
arguments).

Comments, notes?

Regards

Pavel
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index b25b3f1..304fc91 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -51,6 +51,8 @@ bool		plpgsql_check_syntax = false;
 
 PLpgSQL_function *plpgsql_curr_compile;
 
+PLpgSQL_directives *plpgsql_directives;
+
 /* A context appropriate for short-term allocs during compilation */
 MemoryContext plpgsql_compile_tmp_cxt;
 
@@ -83,6 +85,11 @@ static const ExceptionLabelMap exception_label_map[] = {
 	{NULL, 0}
 };
 
+PLpgSQL_directives default_directives = {
+	NULL,
+	true,		/* is_function_scope */
+	0			/* no special cursor option */
+};
 
 /* --
  * static prototypes
@@ -374,6 +381,9 @@ do_compile(FunctionCallInfo fcinfo,
 	plpgsql_DumpExecTree = false;
 	plpgsql_start_datums();
 
+	/* set default compile directives */
+	plpgsql_directives = _directives;
+
 	switch (function->fn_is_trigger)
 	{
 		case PLPGSQL_NOT_TRIGGER:
@@ -852,6 +862,9 @@ plpgsql_compile_inline(char *proc_source)
 	plpgsql_DumpExecTree = false;
 	plpgsql_start_datums();
 
+	/* set default compile directives */
+	plpgsql_directives = _directives;
+
 	/* Set up as though in a function returning VOID */
 	function->fn_rettype = VOIDOID;
 	function->fn_retset = false;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b48146a..66b3ce9 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3625,7 +3625,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
 	{
 		ListCell   *l;
 
-		exec_prepare_plan(estate, expr, 0);
+		exec_prepare_plan(estate, expr, expr->cursor_options);
 		stmt->mod_stmt = false;
 		foreach(l, SPI_plan_get_plan_sources(expr->plan))
 		{
@@ -4366,7 +4366,7 @@ exec_assign_expr(PLpgSQL_execstate *estate, PLpgSQL_datum *target,
 	 */
 	if (expr->plan == NULL)
 	{
-		exec_prepare_plan(estate, expr, 0);
+		exec_prepare_plan(estate, expr, expr->cursor_options);
 		if (target->dtype == PLPGSQL_DTYPE_VAR)
 			exec_check_rw_parameter(expr, target->dno);
 	}
@@ -5173,7 +5173,7 @@ exec_eval_expr(PLpgSQL_execstate *estate,
 	 * If first time through, create a plan for this expression.
 	 */
 	if (expr->plan == NULL)
-		exec_prepare_plan(estate, expr, 0);
+		exec_prepare_plan(estate, expr, expr->cursor_options);
 
 	/*
 	 * If this is a simple expression, bypass SPI and use the executor
@@ -5252,8 +5252,8 @@ exec_run_select(PLpgSQL_execstate *estate,
 	 * On the first call for this expression generate the plan
 	 */
 	if (expr->plan == NULL)
-		exec_prepare_plan(estate, expr, parallelOK ?
-		  CURSOR_OPT_PARALLEL_OK : 0);
+		exec_prepare_plan(estate, expr, (parallelOK ?
+		  CURSOR_OPT_PARALLEL_OK : 0) | expr->cursor_options);
 
 	/*
 	 * If a portal was requested, put the query into the portal
diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index 906fe01..65fea0e 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -1535,6 +1535,10 @@ static void
 dump_expr(PLpgSQL_expr *expr)
 {
 	printf("'%s'", expr->query);
+	if (expr->cursor_options & CURSOR_OPT_GENERIC_PLAN)
+		printf("/* GENERIC_PLAN! */");
+	if (expr->cursor_options & CURSOR_OPT_CUSTOM_PLAN)
+		printf("/* CUSTOM_PLAN! */");
 }
 
 void
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 4a4cd6a..3f236b5 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -108,6 +108,9 @@ static	PLpgSQL_expr	

Re: [HACKERS] Microvacuum support for Hash Index

2017-01-23 Thread Jesper Pedersen

Hi Ashutosh,

On 01/20/2017 03:24 PM, Jesper Pedersen wrote:

Yeah, those are the steps; just with a Skylake laptop.

However, I restarted with a fresh master, with WAL v8 and MV v5, and
can't reproduce the issue.



I have done some more testing with this, and have moved to the patch 
back to 'Needs Review' pending Amit's comments.


Best regards,
 Jesper



--
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] Undefined psql variables

2017-01-23 Thread Tom Lane
"David G. Johnston"  writes:
> On Mon, Jan 23, 2017 at 11:16 AM, Fabien COELHO  wrote:
>> Currently the value of a non existing psql-variable is... its own
>> reference:-(
>> 
>> psql> \echo :x
>> :x
>> 
>> I'm not sure of the rational, apart from the probable lexer implementation
>> point of view. Maybe an empty string or 0 or some configurable value would
>> provide better alternative.

> The fundamental problem is that:
> SELECT 'testing' AS ":tablename"
> is perfectly valid SQL code.

Yeah, but psql does know not to try to resolve :something inside a quoted
literal or identifier.  The actual problem is with constructs like

SELECT somearray[lower:upper] FROM ...

If the user is thinking that's an array subscript not a variable
reference, we don't want to break their query when we don't even have
a useful thing to contribute.

Back in the day, PG allowed ":" as a generic operator name, making
this even worse; but I think the only remaining SQL syntax that could
include a colon is array slicing.

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] Generic type subscription

2017-01-23 Thread Tom Lane
I wrote:
> ... (If that means
> we use two functions not one per datatype, that's fine with me.)

Actually, after thinking about that a bit more: you've really squeezed
*three* different APIs into one function.  Namely, subscript-reference
parse analysis, array subscripting execution, and array assignment
execution.  It would be cleaner, and would reduce runtime overhead a bit,
if those were embodied as three separate functions.

It might be possible to get away with having only one pg_type column,
pointing at the parse-analysis function.  That function would generate
a SubscriptingRef tree node containing the OID of the appropriate
execution function, which execQual.c could call without ever knowing
its name explicitly.

This clearly would work for built-in types, since the parse-analysis
function could rely on fmgroids.h for the OIDs of the execution functions.
But I'm less sure if it would work in extensions, which won't have
predetermined OIDs for their functions.  Is there a way for a function
in an extension to find the OID of one of its sibling functions?

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] Online enabling of page level checksums

2017-01-23 Thread David Christensen

> On Jan 23, 2017, at 10:59 AM, David Christensen  wrote:
> 
>> 
>> On Jan 23, 2017, at 10:53 AM, Simon Riggs  wrote:
>> 
>> On 23 January 2017 at 16:32, David Christensen  wrote:
>> 
>>> ** Handling checksums on a standby:
>>> 
>>> How to handle checksums on a standby is a bit trickier since checksums are 
>>> inherently a local cluster state and not WAL logged but we are storing 
>>> state in the system tables for each database we need to make sure that the 
>>> replicas reflect truthful state for the checksums for the cluster.
>> 
>> Not WAL logged? If the objective of this feature is robustness, it
>> will need to be WAL logged.
>> 
>> Relation options aren't accessed by the startup process during
>> recovery, so that part won't work in recovery. Perhaps disable
>> checksumming until the everything is enabled rather than relation by
>> relation.
>> 
>> If y'all serious about this then you're pretty late in the cycle for
>> such a huge/critical patch. So lets think of ways of reducing the
>> complexity...
>> 
>> Seems most sensible to add the "enable checksums for table" function
>> into VACUUM because then it will happen automatically via autovacuum,
>> or you could force the issue using something like
>> vacuumdb --jobs 4 --enable-checksums
>> That gives you vacuum_delay and a background worker for no effort
> 
> I’m not sure that this will work as-is, unless we’re forcing VACUUM to ignore 
> the visibility map.  I had originally considered having this sit on top of 
> VACUUm though, we just need a way to iterate over all relations and read 
> every page.

Another issue with this (that I think would still exist with the bgworker 
approach) is how to handle databases with datallowconn = 0.  `vacuumdb`, at 
least, explicitly filters out these rows when iterating over databases to 
connect to, so while we could enable them for all databases, we can’t enable 
for the cluster without verifying that these disallowed dbs are already 
checksummed.
--
David Christensen
End Point Corporation
da...@endpoint.com
785-727-1171





-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [PATCH] Generic type subscription

2017-01-23 Thread Tom Lane
Dmitry Dolgov <9erthali...@gmail.com> writes:
> [ generic_type_subscription_v6.patch ]

Not too surprisingly, this doesn't apply anymore in the wake of commit
ea15e1867.  Could you rebase?  Changes for that should be pretty trivial
I'd expect.

I took an extremely quick look over the patch --- mostly looking
at the header file changes not the code --- and have a couple of
comments:

1. As I mentioned previously, it's a seriously bad idea that ArrayRef
is used for both array subscripting and array assignment.  Let's fix
that while we're at it, rather than setting that mistake in even more
stone by embedding it in a datatype extension API.

2. I'm not very pleased that the subscripting functions have signature
"subscripting(internal) returns internal"; we have more than enough of
those already, and each one is a hazard for plugging the wrong function
into the wrong place.  Worse yet, you're flat out lying about the number
of arguments that these functions actually expect to receive, which is
something that could get broken by any number of plausible future changes.
Can we arrange to do that differently?  I'd prefer something in which the
argument and result types are visibly connected to the actual datatypes
at hand, for instance
  array_subscript(anyarray, internal) returns anyelement
  array_assign(anyarray, internal, anyelement) returns anyarray
where the "internal" argument is some representation of only the subscript
expressions.  This would allow CREATE TYPE to perform some amount of
checking that the right function(s) had been specified.  (If that means
we use two functions not one per datatype, that's fine with me.)  If that
seems impractical, let's at least pick a signature that doesn't conflict
with any other INTERNAL-using APIs, and preferably has some connection
to what the arguments really are.

3. The contents of ArrayRef are designed on the assumption that the same
typmod and collation values apply to both an array and its elements.
That's okay for standard arrays, but I do not think it holds for every
other container situation.  For example, hstore doesn't have collation
last I checked, but it would likely want to treat its element type as
being text, which does.  So this needs to be generalized.

4. It looks like your work on the node processing infrastructure has been
limited to s/ArrayRef/SubscriptingRef/g, but that's not nearly enough.
SubscriptingRef needs to be regarded as an opportunity to invoke a
user-defined function, which means that it now acts quite a bit like
FuncExpr.  For example, the function-to-be-invoked needs to be checked for
volatility, leakproofness, parallel safety, etc in operations that want to
know about such things.  So check_functions_in_node(), for one, needs to
consider SubscriptingRef, and really you'll have to look at everyplace
that deals with FuncExpr and see if it needs a case for SubscriptingRef
now.  I'd advise adding the OID of the subscripting function to
SubscriptingRef, so that those places don't need to do additional catalog
lookups to get it.

BTW, a different approach that might be worth considering is to say that
the nodetree representation of one of these things *is* a FuncExpr, and
the new magic thing is just that we invent a new CoercionForm value
which causes ruleutils.c to print the expression as a subscripting op.
I'm not quite convinced that that's a good idea --- a "coercion format"
that says "subscript" seems a bit weird --- but it would greatly reduce
the number of places you'd have to touch.

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] Rename pg_switch_xlog to pg_switch_wal

2017-01-23 Thread Stephen Frost
Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 1/17/17 5:03 PM, Robert Haas wrote:
> > Right.  I think a lot of that stuff should also be changed.  If we
> > weren't OK with breaking compatibility, why'd we change pg_xlog ->
> > pg_wal?  If we're not willing to change other things to match, let's
> > revert that change and be done with it.
> 
> For the record, I don't like the name "xlog" either.  It would be nice
> if we could have more consistent and intuitive naming.
> 
> But I don't see any proposals to actually change all uses of "xlog" to
> "wal".  What about program names, command line options, etc.?  If the
> argument is, we changed one thing, we should change the rest, then let's
> see that.  I think that argument itself is flawed, but if that's what
> we're going with, let's see the whole plan.

That is the proposal and what Vladimir is working towards, as I
understand it, but one piece at a time rather than one big huge patch,
as he's already stated elsewhere on this thread.

I don't have any problem with asking for a summary of the exact set of
changes that he's planning to make though.  My understanding is that it
includes changing program names, command line options, etc.

> Moreover, I see we still have the pg_clog directory.  I thought that was
> supposed to be renamed as well, to avoid confusing it with a "log"
> directory.  Surely, we should at least conclude that original chapter
> before going further.

My understanding is that it is planned to be changed also, but it's
backed up behind the sudden hang-up with making progress on the xlog ->
WAL changes.

I agree that we could probably just go ahead and switch over to starting
on the clog changes (there was agreement somewhere about the new name
for that too), but, well, if I was someone watching all of this
discussion, I have to admit I probably wouldn't be too excited starting
on another set of name changes with all of this going on.  Admittedly,
the clog rename is a lot less user-facing and perhaps we should have
started with it, but this is where we're at now.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

2017-01-23 Thread Peter Eisentraut
On 1/17/17 5:03 PM, Robert Haas wrote:
> Right.  I think a lot of that stuff should also be changed.  If we
> weren't OK with breaking compatibility, why'd we change pg_xlog ->
> pg_wal?  If we're not willing to change other things to match, let's
> revert that change and be done with it.

For the record, I don't like the name "xlog" either.  It would be nice
if we could have more consistent and intuitive naming.

But I don't see any proposals to actually change all uses of "xlog" to
"wal".  What about program names, command line options, etc.?  If the
argument is, we changed one thing, we should change the rest, then let's
see that.  I think that argument itself is flawed, but if that's what
we're going with, let's see the whole plan.

Moreover, I see we still have the pg_clog directory.  I thought that was
supposed to be renamed as well, to avoid confusing it with a "log"
directory.  Surely, we should at least conclude that original chapter
before going further.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


  1   2   >