Re: [HACKERS] Causal reads take II

[Thomas Munro]

On Mon, May 22, 2017 at 6:32 AM, Thomas Munro
 wrote:
> On Mon, May 22, 2017 at 4:10 AM, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>> I'm wondering about status of this patch and how can I try it out?
>
> Hi Dmitry, thanks for your interest.
>
>>> On 3 January 2017 at 02:43, Thomas Munro 
>>> wrote:
>>> The replay lag tracking patch this depends on is in the current commitfest
>>
>> I assume you're talking about this patch [1] (at least it's the only thread
>> where I could find a `replay-lag-v16.patch`)? But `replay lag tracking` was
>> returned with feedback, so what's the status of this one (`causal reads`)?
>
> Right, replay lag tracking was committed.  I'll post a rebased causal
> reads patch later today.

I ran into a problem while doing this, and it may take a couple more
days to fix it since I am at pgcon this week.  More soon.

-- 
Thomas Munro
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] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

[Andres Freund]

On 2017-05-23 22:47:07 -0400, Robert Haas wrote:
> On Mon, May 22, 2017 at 11:42 AM, Andres Freund  wrote:
> > Ooops.
> >
> > Two issues: Firstly, we get a value smaller than seqmin, obviously
> > that's not ok. But even if we'd error out, it'd imo still not be ok,
> > because we have a command that behaves partially transactionally
> > (keeping the seqmax/min transactionally), partially not (keeping the
> > current sequence state at -9).
> 
> I don't really agree that this is broken.

Just a quick clarification question: You did notice that nextval() in S1
after the rollback returned -9, despite seqmin being 0?  I can see
erroring out being acceptable, but returning flat out wrong values?

- Andres


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

[HACKERS] Shortened URLs for commit messages

[Bruce Momjian]

I have written the following sed script to convert regular Postgres
email message URLs to their shorter form for commit messages:

 sed 's;http\(s\?\)://www\.postgresql\.org/message-id/;http\1://postgr.es/m/;gi'

in case this is helpful to anyone.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] [BUGS] Concurrent ALTER SEQUENCE RESTART Regression

[Robert Haas]

On Mon, May 22, 2017 at 11:42 AM, Andres Freund  wrote:
> Ooops.
>
> Two issues: Firstly, we get a value smaller than seqmin, obviously
> that's not ok. But even if we'd error out, it'd imo still not be ok,
> because we have a command that behaves partially transactionally
> (keeping the seqmax/min transactionally), partially not (keeping the
> current sequence state at -9).

I don't really agree that this is broken.  In 9.6, the update appears
to be fully non-transactional, which you could argue is more
consistent, but I don't think I'd agree.  In other cases where we
can't perform an operation transactionally, we call
PreventTransactionChain(), but in 9.6, ALTER SEQUENCE oobounds
MAXVALUE -10 START -10 MINVALUE -1000 INCREMENT BY -1 RESTART seems to
be allowed in a transaction but a subsequent ROLLBACK has no effect,
which seems fairly awful.

I think it's important to keep in mind that nextval() more or less has
to be non-transactional.  From a certain point of view, that's why we
have sequences.  If nextval() didn't notice actions performed by
uncommitted transactions, then sequences wouldn't deliver unique
values; if it blocked waiting to see whether the other transaction
committed and didn't return a value until it either committed or
aborted, then sequences would have terrible performance.  However,
just because nextval() has to be non-transactional doesn't mean that
ALL sequence operations have to be non-transactional.

I don't really know whether the behavior Peter has chosen here is the
best possible choice, so I'm not intending to mount any sort of
vigorous defense of it. However, this thread started with the
complaint about occasional "ERROR:  tuple concurrently updated"
messages; in my opinion, any such behavior in new code is
unacceptable, and now it's been fixed.  "Totally broken" != "not the
behavior I prefer".

-- 
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] Getting server crash after running sqlsmith

[Robert Haas]

On Tue, May 23, 2017 at 9:45 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> Just out of curiosity, what happens if you try it with the attached patch?
>
> Surely that's pretty unsafe?

Yes.  I was just curious to see whether it would work.  I think what
we need to do is teach pqsignal() to block all of the necessary
signals using sa_mask and then remove all of the explicit
blocking/unblocking logic from the signal handlers themselves.  IIUC,
the point of sa_mask is precisely that you want the operating system
to handle the save/restore of the signal mask rather than doing it
yourself in the handler, precisely because doing it in the handler
creates windows at the beginning and end of the handler where the mask
may not be what you want.

In the case of Linux and MacOS, at least, the default behavior (unless
SA_NODEFER is set) is to automatically block the signal currently
being handled, so there's likely no way to blow out the stack during
the brief window before PG_SETMASK() is called.  You could
receive some *other* signal during that window, but then that one
would blocked too, so I don't think you can stack up more frames this
way than the number of distinct signal handlers you have.  However,
the window at the end of the function - after PG_SETMASK()
has been invoked - can recurse arbitrarily deep.  At that point we've
unblocked the signal we're currently handling, so we're playing with
fire.

-- 
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] PG 10 release notes

[Euler Taveira]

2017-05-04 22:26 GMT-03:00 Andres Freund :

> At least I was a bit more optimistic that we'd get a decent json plugin
> into care soon-ish.  While there was some initial work towards that, it
> unfortunately stalled at some point.
>
> That was my initial idea but there wasn't some interest at that time.
wal2json is used in some replication solutions but also for audit and
change capture solution. I recently received some patches/suggestions from
Daniele Varrazzo [1].


> Euler, are you perchance interested in working towards that? ;)
>

Sure. I can submit it for the next CF.


[1]
https://www.postgresql.org/message-id/CA%2Bmi_8bJ_uPr67j-6mbin537DVvfk%3DbOhmWneyBRfbZu89q0tw%40mail.gmail.com


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

[HACKERS] FW: PGBuildfarm member peripatus Branch REL9_2_STABLE Status changed from PLCheck-C failure to OK

[Larry Rosenman]

Rebuilding Perl and all it’s related ports fixed it. 

Dealing with the FreeBSD folks on what all we (FreeBSD) need to put in 
/usr/ports/UPDATING and / or 
/usr/src/UPDATING


-- 
Larry Rosenman http://www.lerctr.org/~ler
Phone: +1 214-642-9640 E-Mail: l...@lerctr.org
US Mail: 17716 Limpia Crk, Round Rock, TX 78664-7281
 
 

On 5/23/17, 4:32 PM, "buildfarm-adm...@postgresql.org" 
 wrote:

The PGBuildfarm member peripatus had the following event on branch 
REL9_2_STABLE:
Status changed from PLCheck-C failure to OK
The snapshot timestamp for the build is: 2017-05-23 21:27:41
The specs of this machine are:
OS:  FreeBSD / HEAD
Arch: X86_64
Comp: clang / 4.0.0
For more information, see 
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=peripatus=REL9_2_STABLE






-- 
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] Tightening isspace() tests where behavior should match SQL parser

[Tom Lane]

Heikki Linnakangas  writes:
> On 05/20/2017 01:48 PM, Tom Lane wrote:
>> Attached is a proposed patch.  I'm vacillating on whether to
>> back-patch this --- it will fix a reported bug, but it seems
>> at least somewhat conceivable that it will also break cases
>> that were working acceptably before.  Thoughts?

> +1 for back-patching. If I understand correctly, it would change the 
> behavior when you pass a string with non-ASCII leading or trailing 
> whitespace characters to those functions. I suppose that can happen, but 
> it's only pure luck if the current code happens to work in that case. 

Well, it'd work properly for e.g. no-break space in LATINn.  But that
seems like a very narrow use-case, and probably not enough to justify
the misbehavior you might get in multi-byte character sets.

A compromise possibly worth considering is to apply isspace() only in
single-byte encodings.  I think that's more complication than it's
worth, but others might think differently.

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] Default Partition for Range

[Beena Emerson]

Hello,

On Mon, May 22, 2017 at 11:29 AM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Mon, May 22, 2017 at 7:27 AM, Beena Emerson 
> wrote:
> Would it be more readable if this reads as NOT
> (constraint_for_partition_1 || constraint_for_partition_2 ||
> constraint_for_partition_3)? That way, the code to create
> constraint_for_partition_n can be reused and there's high chance that
> we will end up with consistent constraints?
>

PFA the patch which gives the default partition constraint as you have
suggested.


>
> >
> > It still needs more work:
> > 1. Handling addition of new partition after default, insertion of data,
> few
> > more bugs
> > 2. Documentation
> > 3. Regression tests
> >
>
> I think, the default partition support for all the strategies
> supporting it should be a single patch since most of the code will be
> shared?
>
>
Dependency on list default patch:
gram.y : adding the syntax
partition.c:
- default_index member in PartitionBoundInfoData;
- check_new_partition_bound : the code for adding a partition after default
has been completely reused.
- isDefaultPartitionBound function is used.

The structures are same  but the handling of list and range is very
different and the code mostly has  the switch case construct to handle the
2 separately. I feel it could be taken separately.

As suggested in the default list thread, I have created
a partition_bound_has_default macro and avoided usage of has_default in
range code. This has to be used for list as well.
Another suggestion for list which has to be implemented in this patch in
removal of PARTITION_DEFAULT. Ii have not done this in this version.

-- 

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


default_range_partition.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] pgbench more operators & functions

[Pavel Stehule]

Hi

I am watching this patch - it looks so there are not problems. I found only
issue in documentation - new CASE expression is not documented.

Regards

Pavel

[HACKERS] FW: PGBuildfarm member peripatus Branch REL9_2_STABLE Failed at Stage PLCheck-C

[Larry Rosenman]

Looks like the upgrade of this machine to the inode64 commit of FreeBSD busted 
stuff. 

I’m rebuilding perl and all the ports to see if that fixes it. 



-- 
Larry Rosenman http://www.lerctr.org/~ler
Phone: +1 214-642-9640 E-Mail: l...@lerctr.org
US Mail: 17716 Limpia Crk, Round Rock, TX 78664-7281
 
 

On 5/23/17, 1:26 PM, "buildfarm-adm...@postgresql.org" 
 wrote:

The PGBuildfarm member peripatus had the following event on branch 
REL9_2_STABLE:
Failed at Stage: PLCheck-C
The snapshot timestamp for the build is: 2017-05-23 18:20:01
The specs of this machine are:
OS:  FreeBSD / HEAD
Arch: X86_64
Comp: clang / 4.0.0
For more information, see 
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=peripatus=REL9_2_STABLE






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

[HACKERS] unreachable code in partition.c

[Jeevan Ladhe]

Hi,

Following code in function get_qual_for_list(partition.c) is not reachable.

else
result = list_make1(opexpr);

Attached is the patch that removes this dead code.

Regards,
Jeevan Ladhe


partition_remove_dead_code.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] walsender & parallelism

[Petr Jelinek]

On 23/05/17 19:45, Andres Freund wrote:
> 
> 
> On May 23, 2017 1:42:41 PM EDT, Petr Jelinek  
> wrote:
>> Hi,
>>
>> so this didn't really move anywhere AFAICS, do we think the approach
>> I've chosen is good or do we want to do something else here?
> 
> Can you add it to the open items list?
> 

Done

-- 
  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] Why does logical replication launcher set application_name?

[Petr Jelinek]

On 20/04/17 21:33, Peter Eisentraut wrote:
> On 4/18/17 13:18, Tom Lane wrote:
>> I think you're thinking about it wrong.  To my mind the issue is that
>> there should be some generic way to determine that a bgworker process
>> is or is not laboring on behalf of an identifiable user.  It's great
>> that we can tell which user it is when there is one, but clearly some
>> bgworkers will be providing general services that aren't associated with
>> a single user.  So it should be possible to set the userID to zero or
>> some such when the bgworker is one that isn't associated with a
>> particular user.  Maybe the owning user needs to become an additional
>> parameter passed in struct BackgroundWorker.
> 
> I think this is probably a problem particular to the logical replication
> launcher.  Other background workers either do work as a particular user,
> as you say, or don't touch the database at all.  So a localized hack or
> a simple hide-the-user flag might suffice for now.
> 

But that still leaves the application_name issue. My proposal in general
would be to add boolean that indicates that the worker is not using
specific user (this can be easily set in
InitializeSessionUserIdStandalone()) and will work for multiple things.

About application_name, perhaps we should just add bgw_type or bgw_group
and show it as worker_type in activity and that's it?

I think this should be open item btw so I'll add it.

-- 
  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] walsender & parallelism

[Andres Freund]

On May 23, 2017 1:42:41 PM EDT, Petr Jelinek  
wrote:
>Hi,
>
>so this didn't really move anywhere AFAICS, do we think the approach
>I've chosen is good or do we want to do something else here?

Can you add it to the open items list?

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.


-- 
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] walsender & parallelism

[Petr Jelinek]

Hi,

so this didn't really move anywhere AFAICS, do we think the approach
I've chosen is good or do we want to do something else here?

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

[HACKERS] Broken hint bits (freeze)

[Dmitriy Sarafannikov]

Hi hackers,

We have some problems on our production with hint bits and frozen tuples.
More and more following errors began to appear on master after switchover:
ERROR:  58P01: could not access status of transaction 1952523525
DETAIL:  Could not open file "pg_clog/0746": No such file or directory.
LOCATION:  SlruReportIOError, slru.c:896

We investigated the problem with pageinspect and found the tuples that are the 
cause:

xdb311g(master)=# select * from mytable where ctid = '(4,21)';
ERROR:  58P01: could not access status of transaction 1951521353
DETAIL:  Could not open file "pg_clog/0745": No such file or directory.
LOCATION:  SlruReportIOError, slru.c:896

But the same query successfully executed on replica.

We found some difference in hint bits between master and replica:

xdb311g(master)=# SELECT t_xmin, t_infomask::bit(32) & X'0300'::int::bit(32) 
FROM heap_page_items(get_raw_page(‘mytable',4)) where lp=21;
-[ RECORD 1 ]--
t_xmin   | 1951521353
?column? | 

old master, now replica:
xdb311e(replica)=# SELECT t_xmin, t_infomask::bit(32) & X'0300'::int::bit(32) 
FROM heap_page_items(get_raw_page(‘mytable',4)) where lp=21;
-[ RECORD 1 ]--
t_xmin   | 1951521353
?column? | 0011

X’0300’ = HEAP_XMIN_FROZEN according to

#define HEAP_XMIN_COMMITTED 0x0100  /* t_xmin committed */
#define HEAP_XMIN_INVALID   0x0200  /* t_xmin invalid/aborted */
#define HEAP_XMIN_FROZEN(HEAP_XMIN_COMMITTED|HEAP_XMIN_INVALID)

xdb311g(master)=# select relfrozenxid from pg_class where relname = ‘mytable';
relfrozenxid
--
2266835605
(1 row)

This tuple must be frozen but there are no set bits HEAP_XMIN_COMMITTED and 
HEAP_XMIN_INVALID on master

Another interesting thing that LSN of this page on master and replica are not 
the same:
xdb311g(master)=# select lsn from page_header(get_raw_page(‘mytable',4));
   lsn
---
8092/6A26DD08
(1 row)

xdb311e(replica)=# select lsn from page_header(get_raw_page(‘mytable',4));
   lsn
---
838D/C4A0D280
(1 row)

And LSN on replica is greater that LSN on master (838D/C4A0D280 > 8092/6A26DD08)
How can this be possible?

We wrote a query which returns ctid of frozen tuples, which must be frozen but 
not actually frozen.

xdb311e(replica)=# select t_ctid from generate_series(0, 
pg_relation_size(‘mytable')/8192 - 1 ) s(i) left join lateral 
heap_page_items(get_raw_page(‘mytable',s.i::int)) on true where 
t_xmin::text::bigint < (select relfrozenxid::text::bigint from pg_class where 
relname = ‘mytable') and t_infomask & X'0300'::int < 1;
t_ctid
---
(400,16)
(2837,71)
(2837,72)
(2837,73)
(2837,75)
(2837,76)
(3042,40)
(4750,80)
(4750,81)
(5214,60)
(5214,65)
(6812,31)
(6912,63)
(7329,8)
(7374,26)
(7374,27)
(16 rows)
Same query on master returns 317 rows.

Our thoughts:
1) We think that it is related to switchover.
2) Any WAL-logged modification of this page on master will replace this page on 
replica due to full page writes.
 And all replicas will have broken hint bits too. It’s dangerous.

Where to dig further?

RHEL6, PostgreSQL 9.6.3, wal_log_hints=off, full_page_writes=on, fsync=on, 
checksums disabled.
We don’t think that it is any hardware-related problems because this databases 
started from 9.4
and they survived 2 upgrades with pg_upgrade. And any hardware-related problems 
was not detected. 
Problem appears not only in this shard.
Size of each shard is around 5TB and we can’t provide data.

Regards
Dmitriy Sarafannikov

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

[HACKERS] Seekable compressed TOAST [POC]

[Sokolov Yura]

Good day, everyone.

It is just proposal.
Main concern: allow compressed toast to be seekable.
Since every chunk compressed separately, toast_fetch_datum_slice can
fetch each slice separately as with EXTERNAL storage.

Attached patch is couple of new column storage types:
- EXTSEEKABLE - like external, but every chunk is separately compressed,
- SEEKABLE - mix of MAIN and EXTSEEKABLE, ie values less than 2k acts as 
MAIN

  storage, and greater as EXTSEEKABLE.

I tested it with source code of postgresql (tables with filename and 
content)

EXTENDED storage: 1296k + 15032k = 16328k
EXTERNAL storage:  728k + 44552k = 45280k
EXTSEEKABLE:   728k + 23096k = 23824k
SEEKABLE:  768k + 23072k = 23640k


Patch is not complete: toast_pointer looks like uncompressed, so
toast_datum_size (and so that pg_column_size) reports uncompressed size
of datum.

And certainly it is just POC, cause better scheme could exist.

For example, improved approach could be:
- modify compression function, so it could stop when it produce desired 
amount

  of compressed data,
- instead of (oid, counter, chunk) use (oid, offset_in_uncompressed, 
chunk)

  for toast tuple, so that it could be located fast.
- using modified compression function, make chunks close to current 2k 
limit
  after compression, but compressed separately, and insert them with 
offset

  in uncompressed varlena.

Other improvement could be building dictionary common for all chunks, 
and

storing it in chunk numbered -1.

PS. Interesting result with tsvector of source code:
EXTENDED:
896k + 16144k = 17040k
EXTERNAL:
896k + 16248k = 17144k
EXTSEEKABLE:
896k + 15792k = 16688k
SEEKABLE:
952k + 15752k = 16704k

So, a) looks like tsvector is almost uncompressible (so probably default
storage should be EXTERNAL), b) it is compressed better by chunks.

--
Sokolov Yura aka funny_falcon
Postgres Professional: https://postgrespro.ru
The Russian Postgres CompanyFrom b1bf73d8111b0620558542c34098e0fee5d19b58 Mon Sep 17 00:00:00 2001
From: Sokolov Yura aka funny_falcon 
Date: Mon, 22 May 2017 11:25:51 +0300
Subject: [PATCH] attr storage SEEKABLE and EXTSEEKABLE

EXTSEEKABLE is like EXTERNAL, but every chunk is compressed, so it is still
seekable, but compressed.
SEEKABLE is mix of EXTSEEKABLE and MAIN: if value is small, then it first
compressed and could be saved inline then.
---
 src/backend/access/heap/tuptoaster.c| 78 -
 src/backend/commands/tablecmds.c|  8 +++
 src/backend/commands/typecmds.c |  4 ++
 src/backend/replication/logical/reorderbuffer.c |  9 +++
 src/bin/pg_dump/pg_dump.c   | 10 
 5 files changed, 93 insertions(+), 16 deletions(-)

diff --git a/src/backend/access/heap/tuptoaster.c b/src/backend/access/heap/tuptoaster.c
index aa5a45d..76eac9d 100644
--- a/src/backend/access/heap/tuptoaster.c
+++ b/src/backend/access/heap/tuptoaster.c
@@ -69,7 +69,7 @@ typedef struct toast_compress_header
 
 static void toast_delete_datum(Relation rel, Datum value, bool is_speculative);
 static Datum toast_save_datum(Relation rel, Datum value,
- struct varlena * oldexternal, int options);
+ struct varlena * oldexternal, int options, bool compress);
 static bool toastrel_valueid_exists(Relation toastrel, Oid valueid);
 static bool toastid_valueid_exists(Oid toastrelid, Oid valueid);
 static struct varlena *toast_fetch_datum(struct varlena * attr);
@@ -734,7 +734,7 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
 
 	/*
 	 * Look for attributes with attstorage 'x' to compress.  Also find large
-	 * attributes with attstorage 'x' or 'e', and store them external.
+	 * attributes with attstorage 'x', 'e', 'z' or 's', and store them external.
 	 */
 	while (heap_compute_data_size(tupleDesc,
   toast_values, toast_isnull) > maxDataLen)
@@ -755,7 +755,8 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
 continue;		/* can't happen, toast_action would be 'p' */
 			if (VARATT_IS_COMPRESSED(DatumGetPointer(toast_values[i])))
 continue;
-			if (att[i]->attstorage != 'x' && att[i]->attstorage != 'e')
+			if (att[i]->attstorage != 'x' && att[i]->attstorage != 'e' &&
+	att[i]->attstorage != 'z' && att[i]->attstorage != 's')
 continue;
 			if (toast_sizes[i] > biggest_size)
 			{
@@ -795,7 +796,7 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
 		}
 		else
 		{
-			/* has attstorage 'e', ignore on subsequent compression passes */
+			/* has attstorage 'e' or 'z', ignore on subsequent compression passes */
 			toast_action[i] = 'x';
 		}
 
@@ -813,7 +814,9 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
 			old_value = toast_values[i];
 			toast_action[i] = 'p';
 			toast_values[i] = toast_save_datum(rel, toast_values[i],
-			   toast_oldexternal[i], options);
+			   toast_oldexternal[i], options,
+			   

Re: [HACKERS] Getting server crash after running sqlsmith

[Tom Lane]

Robert Haas  writes:
> Just out of curiosity, what happens if you try it with the attached patch?

Surely that's pretty unsafe?

regards, tom lane


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

Re: [HACKERS] Proposal : For Auto-Prewarm.

[Mithun Cy]

Thanks, Andres,

I have tried to fix all of your comments. One important change has
happened with this patch is previously we used to read one block info
structure at a time and load it. But now we read all of them together
and load it into as DSA and then we distribute the block infos to the
subgroups to load corresponding blocks. With this portability issue
which I have mentioned above will no longer exists as we do not store
any map tables within the dump file.

On Thu, Apr 6, 2017 at 4:12 AM, Andres Freund  wrote:
> On 2017-03-13 18:45:00 +0530, Mithun Cy wrote:
>> launched only after previous one is finished. All of this will only
>> start if the database has reached a consistent state.
>
> Hm. For replay performance it'd possibly be good to start earlier,
> before reaching consistency.  Is there an issue starting earlier?

Earlier patches used to start loading blocks before reaching a
consistent state. Then Robert while reviewing found a basic flaw in my
approach.  The function DropRelFileNodesAllBuffers do not expect
others to load the blocks concurrently while it is getting rid of
buffered blocks. So has to delay loading until database reaches
consistent state so that we can connect to each database and take a
relation lock before loading any of theirs blocks.

>> + * -- Automatically prewarm the shared buffer pool when server restarts.
>
> Don't think we ususally use -- here.

-- Fixed.


>> + *   Copyright (c) 2013-2017, PostgreSQL Global Development Group
>
> Hm, that's a bit of a weird date range.

-- changed it to 2016-2017 is that right?


> The pg_prewarm.c in there looks like some search & replace gone awry.

-- sorry Fixed.


>> +#include "utils/rel.h"
>> +#include "port/atomics.h"
>
> I'd rather just sort these alphabetically.
> I think this should rather be in the initial header.

-- Fixed as suggested and have moved everything into a header file pg_prewarm.h.


> s/until there is a/until there is no/?

-- Fixed.

>
>> +/*
>> + * 
>> 
>> + * ===SIGNAL HANDLERS
>> ===
>> + * 
>> 
>> + */
>
> Hm...

-- I have reverted those cosmetic changes now.

>
>> +static void sigtermHandler(SIGNAL_ARGS);
>> +static void sighupHandler(SIGNAL_ARGS);
>
> I don't think that's a casing we commonly use.  We mostly use CamelCase
> or underscore_case.
>

-- Fixed with CamelCase.


>> + *   Signal handler for SIGUSR1.
>> + */
>> +static void
>> +sigusr1Handler(SIGNAL_ARGS)

> Hm, what's this one for?

-- The prewarm sub-workers will notify with SIGUSR1 on their
startup/shutdown. Updated the comments.

>> +/*
>> + * Shared state information about the running autoprewarm bgworker.
>> + */
>> +typedef struct AutoPrewarmSharedState
>> +{
>> + pg_atomic_uint32 current_task;  /* current tasks performed by
>> +
>>   * autoprewarm workers. */
>> +} AutoPrewarmSharedState;
>
> Hm.  Why do we need atomics here?  I thought there's no concurrency?

There are 3 methods in autoprewarm.
A: prealoaded prewarm bgworker which also prewarm and then periodic dumping;
B: bgwoker launched by launch_autoprewarm_dump() which do the periodic dumping.
C: Immediate dump by backends.

We do not want 2 bgworkers started and working concurrently. and do
not want to dump while prewarm task is running. As you have suggested
rewrote a simple logic with the use of a boolean variable.


>> +sort_cmp_func(const void *p, const void *q)
>> +{
>
> rename to blockinfo_cmp?

-- Fixed.

>
> Superflous parens (repeated a lot).

-- Fixed in all places.


> Hm.  Is it actually helpful to store the file as text?  That's commonly
> going to increase the size of the file quite considerably, no?

-- Having in the text could help in readability or if we want to
modify/adjust it as needed. I shall so a small experiment to check how
much we caould save and produce a patch on top of this for same.

>
> But what is this actually used for?  I thought Robert, in
> http://archives.postgresql.org/message-id/CA%2BTgmoa%3DUqCL2mR%2B9WTq05tB3Up-z4Sv2wkzkDxDwBP7Mj_2_w%40mail.gmail.com
> suggested storing the filenode in the dump, and then to use
> RelidByRelfilenode to get the corresponding relation?

-- Fixed as suggested directly calling RelidByRelfilenode now.


>> +load_one_database(Datum main_arg)
>> +{
>
>> + /* check if file exists and open file in read mode. */
>> + snprintf(dump_file_path, sizeof(dump_file_path), "%s", 
>> AUTOPREWARM_FILE);
>> + file = fopen(dump_file_path, PG_BINARY_R);
>> + if (!file)
>> + return; /* No file to load. */
>
> Shouldn't this be an error case?  In which case is it ok for the file to
> be gone after we launched the worker?

-- Yes sorry a mistake. In 

Re: [HACKERS] Improve logical decoding error message (was wal_level > WAL_LEVEL_LOGICAL)

[Andres Freund]

On 2017-05-23 10:49:54 +, Neha Khatri wrote:
> On Tue, 23 May 2017 at 10:55 am, Alvaro Herrera 
> wrote:
> 
> > Neha Khatri wrote:
> > > On Tue, May 23, 2017 at 10:26 AM, Michael Paquier <
> > michael.paqu...@gmail.com
> >
> > > > There is no wal_level higher than logical, so the current sense looks
> > > > perfectly fine to me.
> > >
> > > If there is no wal_level higher than logical, should the following error
> > > message indicate to set it >= logical.
> > >
> > >  select * from
> > > pg_create_logical_replication_slot('regression_slot','test_decoding');
> > >  ERROR:  logical decoding requires wal_level >= logical
> >
> > I think it's purposefully ambiguous to cover a possible future
> > extension.

Right, IIRC that's how this notion started.


> Should documentation also have similar statement and indicate future
> possibility.
> 
> What is the benefit of having it just in error message.

I personally wouldn't do anything here, it doesn't seem an issue.


- 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] Getting server crash after running sqlsmith

[tushar]

On 05/23/2017 06:25 PM, Robert Haas wrote:

Just out of curiosity, what happens if you try it with the attached patch?

Thanks, issue seems to be fixed after applying your patch.

--
regards,tushar
EnterpriseDB  https://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] Getting server crash after running sqlsmith

[Robert Haas]

On Tue, May 23, 2017 at 1:46 AM, tushar  wrote:
> On 03/29/2017 12:06 AM, Tom Lane wrote:
>>
>> Hm ... I don't see a crash here, but I wonder whether you have parameters
>> set that would cause this query to be run as a parallel query?  Because
>> pg_rotate_logfile() is marked as parallel-safe in pg_proc, which seems
>> probably insane.
>
> Well, I am able to see a crash .  Enable "logging_collector=on" in
> postgresql.conf file / restart the server and fire below sql query - 5 or 6
> times

Just out of curiosity, what happens if you try it with the attached patch?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


no-sigmask.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] Regarding Postgres Dynamic Shared Memory (DSA)

[Mahi Gurram]

Hi Michael,

Thanks for your response.

All i'm building is In-Memory Index as an extension over Postgres.

Postgres Indexes will get Insert calls and Read calls from various
processes(typically client/connection process - forked processes to
postmaster process). Hence i have to maintain my In-Memory index in shared
memory.

If i create DynamicSharedArea (DSA) in postmaster/main process, all these
Client/Connection processes(In-Memory Index Processes)  need not attach to
that DSA using area handle. Because these are forked processes to
postmaster/Main process and hence they automatically gets attached.

Hence i'm trying to create DSA in _PG_init function as it is called by
postmaster/main process.

Hope this is clear.

Thanks & Best Regards,
- Mahi


On Tue, May 23, 2017 at 5:30 PM, Michael Paquier 
wrote:

> On Tue, May 23, 2017 at 6:42 AM, Mahi Gurram  wrote:
> > I'm building In-Memory index extension for Postgres, for which i'm
> trying to
> > use DSA. But ended with some issues, as it is not allowing me to create
> > DSA(Dynamic Shared Area) in _PG_init function.
> >
> > Please refer my_PG_init code below:
> >>
> >> void
> >> _PG_init(void)
> >> {
> >> area = dsa_create(LWLockNewTrancheId(), "CustomIndex_DSA");
> >> area_handle = dsa_get_handle(area);
> >> }
> >
> >
> > Because of this code, Postgres is not starting. Not even giving any error
> > messages in pg logs. Hence, i'm totally clue less :(
>
> It seems to me that you are creating those too early. For example, for
> a background worker process, DSA segments would likely be created in
> the main process routine. Without understanding what you are trying to
> achieve, it is hard to make a good answer. You could always use a
> ramdisk, but that would be likely be a waste of memory as Postgres has
> its own buffer pool, killing the performance gains of OS caching.
> --
> Michael
>

retry shm attach for windows (WAS: Re: [HACKERS] OK, so culicidae is *still* broken)

[Amit Kapila]

On Sat, May 20, 2017 at 5:56 PM, Noah Misch  wrote:
> On Sat, Apr 15, 2017 at 02:30:18PM -0700, Andres Freund wrote:
>> On 2017-04-15 17:24:54 -0400, Tom Lane wrote:
>> > Andres Freund  writes:
>> > > On 2017-04-15 17:09:38 -0400, Tom Lane wrote:
>> > >> Why doesn't Windows' ability to map the segment into the new process
>> > >> before it executes take care of that?
>> >
>> > > Because of ASLR of the main executable (i.e. something like PIE).
>> >
>> > Not following.  Are you saying that the main executable gets mapped into
>> > the process address space immediately, but shared libraries are not?
>
> At the time of the pgwin32_ReserveSharedMemoryRegion() call, the child process
> contains only ntdll.dll and the executable.
>
>> Without PIE/ASLR we can somewhat rely on pgwin32_ReserveSharedMemoryRegion
>> to find the space that PGSharedMemoryCreate allocated still unoccupied.
>
> I've never had access to a Windows system that can reproduce the fork
> failures.  My best theory is that antivirus or similar software injects an
> additional DLL at that early stage.
>
>> > I wonder whether we could work around that by just destroying the created
>> > process and trying again if we get a collision.  It'd be a tad
>> > inefficient, but hopefully collisions wouldn't happen often enough to be a
>> > big problem.
>>
>> That might work, although it's obviously not pretty.
>
> I didn't like that idea when Michael proposed it in 2015.  Since disabling
> ASLR on the exe proved insufficient, I do like it now.  It degrades nicely; if
> something raises the collision rate from 1% to 10%, that just looks like fork
> latency degrading.
>

So it seems both you and Tom are leaning towards some sort of retry
mechanism for shm reattach on Windows.  I also think that is a viable
option to negate the impact of ASLR.  Attached patch does that.  Note
that, as I have mentioned above I think we need to do it for shm
reserve operation as well.  I think we need to decide how many retries
are sufficient before bailing out.  As of now, I have used 10 to have
some similarity with PGSharedMemoryCreate(), but we can choose some
different count as well.  One might say that we can have "number of
retries" as a guc parameter, but I am not sure about it, so not used.
Another point to consider is that do we want the same retry mechanism
for EXEC_BACKEND builds (function
PGSharedMemoryReAttach is different on Windows and EXEC_BACKEND
builds).  I think it makes sense to have retry mechanism for
EXEC_BACKEND builds, so done that way in the patch.  Yet another point
which needs some thought is for reattach operation, before retrying do
we want to reserve the shm by using VirtualAllocEx?

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


win_shm_retry_reattach_v1.patch
Description: Binary data

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

Re: [HACKERS] Increasing parallel workers at runtime

[Rafia Sabih]

On Tue, May 23, 2017 at 4:41 PM, Amit Kapila  wrote:

> Isn't this point contradictory?  Basically, on one side you are
> suggesting to calculate additional workers (work_remaining) based on
> selectivity and on another side you are saying that it will fix
> estimation errors.  IIUC, then you are talking about some sort of
> executor feedback to adjust a number of workers which might be a good
> thing to do but I that is a different problem to solve.  As of now, I
> don't think we have that type of mechanism even for non-parallel
> execution.
>
Yes, I am talking of a executor feedback mechanism sort of a thing.
For non parallel execution it's a lot more challenging since the
execution plan might need to be changed in the runtime, but in
parallel query case only addition/subtraction of workers is to be
dealt, which appears to be a less complex thing.
Why I am thinking in this direction is because in my experience it
seems very easy to regress with more workers when over-estimation is
done and this runtime calculation of workers can mitigate it largely.
However, I understand that this might require more proof than my
experience alone. Let's see if anybody else shares my gut feeling. :)

-- 
Regards,
Rafia Sabih
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] Regarding Postgres Dynamic Shared Memory (DSA)

[Michael Paquier]

On Tue, May 23, 2017 at 6:42 AM, Mahi Gurram  wrote:
> I'm building In-Memory index extension for Postgres, for which i'm trying to
> use DSA. But ended with some issues, as it is not allowing me to create
> DSA(Dynamic Shared Area) in _PG_init function.
>
> Please refer my_PG_init code below:
>>
>> void
>> _PG_init(void)
>> {
>> area = dsa_create(LWLockNewTrancheId(), "CustomIndex_DSA");
>> area_handle = dsa_get_handle(area);
>> }
>
>
> Because of this code, Postgres is not starting. Not even giving any error
> messages in pg logs. Hence, i'm totally clue less :(

It seems to me that you are creating those too early. For example, for
a background worker process, DSA segments would likely be created in
the main process routine. Without understanding what you are trying to
achieve, it is hard to make a good answer. You could always use a
ramdisk, but that would be likely be a waste of memory as Postgres has
its own buffer pool, killing the performance gains of OS caching.
-- 
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] Error-like LOG when connecting with SSL for password authentication

[Michael Paquier]

On Tue, May 23, 2017 at 6:36 AM, Heikki Linnakangas  wrote:
> On 05/22/2017 10:11 PM, Vaishnavi Prabakaran wrote:
>>
>> On Mon, May 22, 2017 at 5:10 PM, Michael Paquier
>> 
>> wrote:
>>
>>> If the protocol version is SSL
>>> 3.0 or TLS 1.0, this result code is returned only if a closure alert
>>> has occurred in the protocol, i.e. if the connection has been closed
>>> cleanly. Note that in this case SSL_ERROR_ZERO_RETURN does not
>>> necessarily indicate that the underlying transport has been closed.
>>
>>
>> I guess this error code exist even for SSL2 protocol, In that case, don't
>> we need to keep the current code for this error code?
>
> If I understand correctly, with SSLv2, SSL_ERROR_ZERO_RETURN does mean that
> the underlying transport has been closed. Returning 0 seems appropriate in
> that case, too.

Am I reading the docs incorrectly then? I understand that with SSLv2
the transport may not be closed after SSL_ERROR_ZERO_RETURN.

> But the point is moot anyway, because PostgreSQL doesn't allow SSLv2
> anymore.

And SSL_OP_NO_SSLv2 is enforced anyway.

Side note.. Looking at the openssl docs, I am just noticing that
SSLv23_method has been marked as deprecated in 1.1.0:
https://www.openssl.org/docs/man1.1.0/ssl/SSLv23_method.html
And has been replaced by TLS_method. Something to keep in mind.
-- 
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] Increasing parallel workers at runtime

[Amit Kapila]

On Mon, May 22, 2017 at 2:54 PM, Rafia Sabih
 wrote:
> On Wed, May 17, 2017 at 2:57 PM, Amit Kapila  wrote:
>> On Tue, May 16, 2017 at 2:14 PM, Ashutosh Bapat
>>  wrote:
>>> On Mon, May 15, 2017 at 9:23 PM, Robert Haas  wrote:
>>>
>>> Also, looking at the patch, it doesn't look like it take enough care
>>> to build execution state of new worker so that it can participate in a
>>> running query. I may be wrong, but the execution state initialization
>>> routines are written with the assumption that all the workers start
>>> simultaneously?
>>>
>>
>> No such assumptions, workers started later can also join the execution
>> of the query.
>>
> If we are talking of run-time allocation of workers I'd like to
> propose an idea to safeguard parallelism from selectivity-estimation
> errors. Start each query (if it qualifies for the use of parallelism)
> with a minimum number of workers (say 2) irrespective of the #planned
> workers. Then as query proceeds and we find that there is more work to
> do, we allocate more workers.
>
> Let's get to the details a little, we'll have following new variables,
> - T_int - a time interval at which we'll periodically check if the
> query requires more workers,
> - work_remaining - a variable which estimates the work yet to do. This
> will use the selectivity estimates to find the total work done and the
> remaining work accordingly. Once, the actual number of rows crosses
> the estimated number of rows, take maximum possible tuples for that
> operator as the new estimate.
>
> Now, we'll check at gather, after each T_int if the work is remaining
> and allocate another 2 (say) workers. This way we'll keep on adding
> the workers in small chunks and not in one go. Thus, saving resources
> in case over-estimation is done.
>
> Some of the things we may improvise upon are,
> - check if we want to increase workers or kill some of them. e.g. if
> the filtering is not happening at estimated at the node, i.e. #output
> tuples is same as #input tuples, then do not add any more workers as
> it will increase the work at gather only.
> - Instead of just having a number of 2 or 4 workers at the start,
> allocate x% of planned workers.
> - As the query progresses, we may alter the value of T_int and/or
> #workers to allocate. e.g. till a query is done something less than
> 50%, check at every T_int, after that increase T_int to T_int(1 + .5),
> similarly for #workers, because now allocating more workers might not
> do much work rather the cost of adding new workers could be more.
>
> This scheme is likely to safeguard parallelism with selectivity
> estimation errors in a sense that it is using resources only when
> required.

Isn't this point contradictory?  Basically, on one side you are
suggesting to calculate additional workers (work_remaining) based on
selectivity and on another side you are saying that it will fix
estimation errors.  IIUC, then you are talking about some sort of
executor feedback to adjust a number of workers which might be a good
thing to do but I that is a different problem to solve.  As of now, I
don't think we have that type of mechanism even for non-parallel
execution.



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


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

Re: [HACKERS] Regarding B-Tree Lookup

[Mahi Gurram]

Thank you all for your responses to help me out.

The below code worked for me... pasting it here (May be helpful for someone)

Relation heap;
> ItemPointer ht_ctid;
> heap = heap_open(50620, AccessShareLock); /* 50620 - Table/Relation Oid -
> Hardcoded for better understanding */

ScanKeyInit(, 1, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum(100));
>  /* 100 is the Value of PK of which i'm doing a lookup*/
> scan = systable_beginscan(heap, 50624 , true, NULL, 1, ); // 50624 is
> the Oid of PKIndex for this Table/Relation.

tuple = systable_getnext(scan);
> if (HeapTupleIsValid(tuple)){
> ht_ctid = >t_self;
> }
> systable_endscan(scan);
> heap_close(heap, AccessShareLock);


Hope this helps for some one.

PS: You have to use  different Procedure(F_INT8EQ/F_TEXTEQ etc..) and Datum
Conversion functions(Int64GetDatum/CStringGetTextDatum etc..) in
ScanKeyInit() function as per your PK Data Types.

Thanks & Best Regards,
- Mahi

On Tue, May 2, 2017 at 5:59 PM, Tom Lane  wrote:

> Michael Paquier  writes:
> > On Tue, May 2, 2017 at 6:12 PM, Mahi Gurram  wrote:
> >> Please suggest me the easiest way to lookup into PK's B-Tree index for
> >> getting TIDs.
>
> > Why don't you just use SPI within your extension? No need to copy the
> > logic for btree lookups this way.
>
> There's not actually that much code needed, though -- basically
> index_beginscan, index_rescan, index_getnext, index_endscan.  One possible
> model to follow is systable_beginscan and friends, in genam.c.
>
> I think that you could possibly get away with just applying
> systable_beginscan to random user tables, even.  But it's at least a
> conceptual mismatch, and there are some things in there, like the
> test on IgnoreSystemIndexes, that you probably don't want.
>
> regards, tom lane
>

Re: [HACKERS] PROVE_FLAGS

[Andrew Dunstan]

On 17 May 2017 at 14:30, Robert Haas  wrote:
> On Tue, May 16, 2017 at 9:20 PM, Andrew Dunstan
>  wrote:
>> Inheriting variables from the environment is a part of make by design.
>> We have PG_PROVE_FLAGS for our own forced settings.
>
> I don't buy this argument.  We've had previous cases where we've gone
> through and added -X to psql invocations in the regression tests
> because, if you don't, some people's .psqlrc files may cause tests to
> fail, which nobody wants.  The more general principle is that the
> regression tests should ideally pass regardless of the local machine
> configuration.  It's proven impractical to make that universally true,
> but that doesn't make it a bad goal.  Now, for all I know it may be
> that setting PROVE_FLAGS can never cause a regression failure, but I
> still tend to agree with Peter that the regression tests framework
> should insulate us from the surrounding environment rather than
> absorbing settings from it.
>

In that case we're going to need to invent a way to do this similarly
in vcregress.pl. I'm not simply going to revert to the situation where
it and the makefiles are completely out of sync on this. The previous
patch was made more or less by ignoring the existence of vcregress.pl.

I will look at this again probably after pgcon. I don't think it's
terribly urgent.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Tightening isspace() tests where behavior should match SQL parser

[Heikki Linnakangas]

On 05/20/2017 01:48 PM, Tom Lane wrote:

Attached is a proposed patch.  I'm vacillating on whether to
back-patch this --- it will fix a reported bug, but it seems
at least somewhat conceivable that it will also break cases
that were working acceptably before.  Thoughts?


+1 for back-patching. If I understand correctly, it would change the 
behavior when you pass a string with non-ASCII leading or trailing 
whitespace characters to those functions. I suppose that can happen, but 
it's only pure luck if the current code happens to work in that case. 
And even if so, it's IMO still quite reasonable to change the behavior, 
on the grounds that the new behavior is what the core SQL lexer would do.


- Heikki



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

Re: [HACKERS] Improve logical decoding error message (was wal_level > WAL_LEVEL_LOGICAL)

[Neha Khatri]

On Tue, 23 May 2017 at 10:55 am, Alvaro Herrera 
wrote:

> Neha Khatri wrote:
> > On Tue, May 23, 2017 at 10:26 AM, Michael Paquier <
> michael.paqu...@gmail.com
>
> > > There is no wal_level higher than logical, so the current sense looks
> > > perfectly fine to me.
> >
> > If there is no wal_level higher than logical, should the following error
> > message indicate to set it >= logical.
> >
> >  select * from
> > pg_create_logical_replication_slot('regression_slot','test_decoding');
> >  ERROR:  logical decoding requires wal_level >= logical
>
> I think it's purposefully ambiguous to cover a possible future
> extension.
>

Should documentation also have similar statement and indicate future
possibility.

What is the benefit of having it just in error message.

Regards,
Neha
-- 
Cheers,
Neha

[HACKERS] Regarding Postgres Dynamic Shared Memory (DSA)

[Mahi Gurram]

Hello everyone,

I'm building In-Memory index extension for Postgres, for which i'm trying
to use DSA. But ended with some issues, as it is not allowing me to create
DSA(Dynamic Shared Area) in _PG_init function.

Please refer my_PG_init code below:

> void
> _PG_init(void)
> {
> area = dsa_create(LWLockNewTrancheId(), "CustomIndex_DSA");
> area_handle = dsa_get_handle(area);
> }


Because of this code, Postgres is not starting. Not even giving any error
messages in pg logs. Hence, i'm totally clue less :(


Please let me know how to proceed. Your help is highly appreciated.


PS: Applied all the DSM related patches over Postgres 9.6.1 as per below
thread and confirmed the same by running some test programs.

https://www.postgresql.org/message-id/flat/CAEepm%3D1z5WLuNoJ80PaCvz6EtG9dN0j-KuHcHtU6QEfcPP5-qA%40mail.gmail.com#CAEepm=1z5wlunoj80pacvz6etg9dn0j-kuhchtu6qefcpp5...@mail.gmail.com


Thanks & Best Regards,

- Mahi

Re: [HACKERS] Error-like LOG when connecting with SSL for password authentication

[Heikki Linnakangas]

On 05/22/2017 10:11 PM, Vaishnavi Prabakaran wrote:

On Mon, May 22, 2017 at 5:10 PM, Michael Paquier 
wrote:


If the protocol version is SSL
3.0 or TLS 1.0, this result code is returned only if a closure alert
has occurred in the protocol, i.e. if the connection has been closed
cleanly. Note that in this case SSL_ERROR_ZERO_RETURN does not
necessarily indicate that the underlying transport has been closed.


I guess this error code exist even for SSL2 protocol, In that case, don't
we need to keep the current code for this error code?


If I understand correctly, with SSLv2, SSL_ERROR_ZERO_RETURN does mean 
that the underlying transport has been closed. Returning 0 seems 
appropriate in that case, too.


But the point is moot anyway, because PostgreSQL doesn't allow SSLv2 
anymore.


- Heikki



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

Re: [HACKERS] Error-like LOG when connecting with SSL for password authentication

[Heikki Linnakangas]

On 05/22/2017 03:10 AM, Michael Paquier wrote:

Hi all,

When attempting to connect using password authentication through SSL,
the backend will complain in its log with the following entry before
calling sendAuthRequest(), which asks the client for a password:
LOG:  could not receive data from client: Connection reset by peer

After a short talk with Heikki, it seems that be_tls_read() complains
on SSL_ERROR_ZERO_RETURN, which is documented here:
https://wiki.openssl.org/index.php/Manual:SSL_get_error(3)
The TLS/SSL connection has been closed. If the protocol version is SSL
3.0 or TLS 1.0, this result code is returned only if a closure alert
has occurred in the protocol, i.e. if the connection has been closed
cleanly. Note that in this case SSL_ERROR_ZERO_RETURN does not
necessarily indicate that the underlying transport has been closed.

As this is a clean shutdown of the SSL connection, shouldn't
be_tls_read() return 0 to the caller instead of -1? This would map
with what the non-SSL code path does for raw reads.


Yeah. The be_tls_read() change looks OK to me.

Can SSL_ERROR_ZERO_RETURN also happen in a write? I suppose it can, but 
returning 0 from secure_write() seems iffy. My man page for send(2) 
doesn't say that it would return 0 if the connection has been closed by 
the peer, so that would be different from the non-SSL codepath. Looking 
at the only caller to secure_write(), it does check for 0, and would 
treat it correctly as EOF, so it would work. But it makes me a bit 
nervous, a comment in secure_write() at least would be in order, to 
point out that it can return 0 to indicate EOF, unlike the raw write(2) 
or send(2) system functions.


In libpq, do we want to set the "SSL connection has been closed 
unexpectedly" message? In the non-SSL case, pqsecure_raw_read() doesn't 
set any error message on EOF. Also, even though the comment in 
pgtls_read() says "... we should not report it as a server crash", 
looking at pqReadData, ISTM that returning 0 will in fact lead to the 
"server closed the connection unexpectedly" message. And it seems like a 
good assumption anyway, that the server did in fact terminate 
abnormally, if that happens. We don't expect the server to disconnect 
the client without notice, even though it's not unusual for the client 
to disconnect without notifying the server.


- Heikki



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

[HACKERS] Index Only Scan support for cube

[Andrew Borodin]

Hi, hackers!

Here's a small patch that implements fetch function necessary for
Index Only Scans that use cube data type.
I reuse function g_cube_decompress() instead of creating new function
g_cube_fetch().
Essentially, they both have to detoast data.

How do you think, is it better to create a shallow copy of
g_cube_decompress instead?
Any other suggestions on the functionality?

This

CREATE TABLE SOMECUBES AS SELECT CUBE(X,X+1) C FROM GENERATE_SERIES(1,100) X;
CREATE INDEX SOMECUBES_IDX ON SOMECUBES USING GIST(C);
SET ENABLE_SEQSCAN = FALSE;
EXPLAIN (COSTS OFF ) SELECT C FROM SOMECUBES WHERE C<@CUBE(30,40);

now produces

Index Only Scan using somecubes_idx on somecubes
  Index Cond: (c <@ '(30),(40)'::cube)

instead of

Index Scan using somecubes_idx on somecubes
  Index Cond: (c <@ '(30),(40)'::cube)



Best regards, Andrey Borodin, Octonica.


cubefetch.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] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

[tushar]

Hi,

ALTER SUBSCRIPTION ..SET PUBLICATION  refresh  is removing all 
the attached subscription('s).


X Machine -

s=# create table t(n int);
CREATE TABLE
s=# create table t1(n int);
CREATE TABLE
s=# create publication pub for table  t,t1;
CREATE PUBLICATION
s=#

Y Machine -

s=# create table t(n int);
CREATE TABLE
s=# create table t1(n int);
CREATE TABLE
s=# create subscription s1 connection 'dbname=s port=5432 user=centos 
host=localhost' publication pub;

NOTICE:  synchronized table states
NOTICE:  created replication slot "s1" on publisher
CREATE SUBSCRIPTION

s=# alter subscription s1 set publication  skip refresh ;
NOTICE:  removed subscription for table public.t
NOTICE:  removed subscription for table public.t1
ALTER SUBSCRIPTION
s=#

I think - this is probably due to not given publication NAME in the sql 
query .


we are doing a syntax check at the time of REFRESH but not with SKIP 
REFRESH


s=# alter subscription s1 set publication   refresh ;
ERROR:  syntax error at or near ";"
LINE 1: alter subscription s1 set publication   refresh ;
^
s=# alter subscription s1 set publication  skip refresh ;
ALTER SUBSCRIPTION
s=#

--
regards,tushar
EnterpriseDB  https://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] Create subscription with `create_slot=false` and incorrect slot name

[Dmitry Dolgov]

On 23 May 2017 at 07:26, Euler Taveira  wrote:
>
> ReplicationSlotValidateName() should be called in
parse_subscription_options() to avoid a pilot error.
> IMHO we should prevent a future error (use of invalid slot name).

Yes, I see now. I assume this little patch should be enough for that.
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 86eb31d..625b5e9 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -39,6 +39,7 @@
 
 #include "replication/logicallauncher.h"
 #include "replication/origin.h"
+#include "replication/slot.h"
 #include "replication/walreceiver.h"
 #include "replication/walsender.h"
 #include "replication/worker_internal.h"
@@ -138,6 +139,9 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
 			*slot_name_given = true;
 			*slot_name = defGetString(defel);
 
+			/* Validate slot_name even if create_slot = false */
+			ReplicationSlotValidateName(*slot_name, ERROR);
+
 			/* Setting slot_name = NONE is treated as no slot name. */
 			if (strcmp(*slot_name, "none") == 0)
 *slot_name = NULL;
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 91ba8ab..6334a18 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -62,6 +62,9 @@ CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpu
 ERROR:  subscription with slot_name = NONE must also set create_slot = false
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
 ERROR:  subscription with slot_name = NONE must also set enabled = false
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = "invalid name", create_slot = false);
+ERROR:  replication slot name "invalid name" contains invalid character
+HINT:  Replication slot names may only contain lower case letters, numbers, and the underscore character.
 -- ok - with slot_name = NONE
 CREATE SUBSCRIPTION testsub3 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, connect = false);
 WARNING:  tables were not subscribed, you will have to run ALTER SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 4b694a3..5b635bc 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -47,6 +47,7 @@ CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpu
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE);
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false);
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = "invalid name", create_slot = false);
 
 -- ok - with slot_name = NONE
 CREATE SUBSCRIPTION testsub3 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, connect = false);

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

Re: [HACKERS] ECPG: pg_type.h file is not synced

[Michael Meskes]

Hi,

> I am looking at ECPG source code. In the "ecpg/ecpglib/pg_type.h" file I
> have seen following comment:
> 
> ** keep this in sync with src/include/catalog/pg_type.h*
> 
> But I think the "ecpg/ecpglib/pg_type.h" file is currently not synced
> with the above file.
> 
> I have added the remaining types in the attached patch.
> 
> I would like to know whether we can add remaining types in the
> "ecpg/ecpglib/pg_type.h" file or not?

It's not too big a deal as neither of the new OIDs is used in ecpg, but
still, the file should be up to date.

Thanks for the patch, committed.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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] Regression in join selectivity estimations when using foreign keys

[David Rowley]

On 22 May 2017 at 16:10, David Rowley  wrote:
> I also just noticed that I don't think I've got ANTI join cases
> correct in the patch I sent. I'll look at that now.

I've attached an updated patch.

This one is much less invasive than my original attempt.

There are two fundamental changes here:

1) OUTER joins now use the foreign key as proof that the join
condition must match.
2) selectivity of nullfrac for null valued columns for OUTER joins is
no longer taken into account. This is now consistent with INNER joins,
which might not be perfect, but it's less surprising. If this is a
problem then we can consider applying something like my 0002 patch
above, however that can mean that nulls are double counted if there
are any other strict clauses which are not part of the foreign key
constraint, so that idea is not perfect either.

In addition to those two things, the poor selectivity estimation in my
original email is also fixed.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


fk_join_est_fix_2017-05-23.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] ECPG: pg_type.h file is not synced

[vinayak]

Hello,

I am looking at ECPG source code. In the "ecpg/ecpglib/pg_type.h" file I 
have seen following comment:


** keep this in sync with src/include/catalog/pg_type.h*

But I think the "ecpg/ecpglib/pg_type.h" file is currently not synced 
with the above file.


I have added the remaining types in the attached patch.

I would like to know whether we can add remaining types in the 
"ecpg/ecpglib/pg_type.h" file or not?


Regards,

Vinayak Pokale

NTT Open Source Software Center



ECPG_pg_type_h.patch
Description: binary/octet-stream

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

Re: [HACKERS] Create subscription with `create_slot=false` and incorrect slot name

[Kuntal Ghosh]

On Tue, May 23, 2017 at 10:56 AM, Euler Taveira  wrote:
> 2017-05-22 17:52 GMT-03:00 Dmitry Dolgov <9erthali...@gmail.com>:
>>
>> Maybe this question was already raised before, but I couldn't find a
>> discussion. When I'm creating a subscription with `create_slot=false`
>> looks
>> like it's possible to pass a slot name with invalid characters. In this
>> particular case both publisher and subscriber were on the same machine:
>>
>> =# CREATE SUBSCRIPTION test_sub CONNECTION 'dbname=db host=host
>> port=port user=user' PUBLICATION test_pub WITH (create_slot=false,
>> slot_name='test slot');
>> NOTICE:  0: synchronized table states
>> LOCATION:  CreateSubscription, subscriptioncmds.c:443
>> CREATE SUBSCRIPTION
>> Time: 5.887 ms
>>
> The command succeed even if slot_name is invalid. That's because slot_name
> isn't validated. ReplicationSlotValidateName() should be called in
> parse_subscription_options() to avoid a pilot error. IMHO we should prevent
> a future error (use of invalid slot name).
>
+1. Since, slot_name can be provided even when create_slot is set
false, it should be validated as well while creating the subscription.



-- 
Thanks & Regards,
Kuntal Ghosh
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