Re: [HACKERS] Problem in Parallel Bitmap Heap Scan?

[Robert Haas]

On Sun, Apr 9, 2017 at 11:17 PM, Noah Misch  wrote:
> The above-described topic is currently a PostgreSQL 10 open item.

I have committed the patch.

-- 
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] Problem in Parallel Bitmap Heap Scan?

[Noah Misch]

On Sat, Mar 25, 2017 at 09:55:21PM +1300, Thomas Munro wrote:
> On Sat, Mar 25, 2017 at 6:04 PM, Amit Kapila  wrote:
> > On Tue, Mar 21, 2017 at 5:51 PM, Dilip Kumar  wrote:
> >> On Tue, Mar 21, 2017 at 4:47 PM, Thomas Munro
> >>  wrote:
> >>> I noticed a failure in the inet.sql test while running the regression
> >>> tests with parallelism cranked up, and can reproduce it interactively
> >>> as follows.  After an spgist index is created and the plan changes to
> >>> the one shown below, the query returns no rows.
> >>
> >> Thanks for reporting.  Seems like we are getting issues related to
> >> TBM_ONE_PAGE and TBM_EMPTY.
> >>
> >> I think in this area we need more testing, reason these are not tested
> >> properly because these are not the natural case for parallel bitmap.
> >> I think in next few days I will test more such cases by forcing the
> >> parallel bitmap.
> >>
> >
> > Okay, is your testing complete?
> >
> >> Here is the patch to fix the issue in hand.  I have also run the
> >> regress suit with force_parallel_mode=regress and all the test are
> >> passing.
> >>
> >
> > Thomas, did you get chance to verify Dilip's latest patch?
> >
> > I have added this issue in PostgreSQL 10 Open Items list so that we
> > don't loose track of this issue.
> 
> The result is correct with this patch.  I ran make installcheck then
> the same steps as above and the query result was correct after
> creating the index.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.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] Problem in Parallel Bitmap Heap Scan?

[Dilip Kumar]

On Sat, Mar 25, 2017 at 2:25 PM, Thomas Munro
 wrote:
>>> I think in this area we need more testing, reason these are not tested
>>> properly because these are not the natural case for parallel bitmap.
>>> I think in next few days I will test more such cases by forcing the
>>> parallel bitmap.
>>>
>>
>> Okay, is your testing complete?

Yes, I have done more testing around this area with more cases, like
one page with BitmapOr etc.
Now it looks fine to me.
>>
>>> Here is the patch to fix the issue in hand.  I have also run the
>>> regress suit with force_parallel_mode=regress and all the test are
>>> passing.
>>>
>>
>> Thomas, did you get chance to verify Dilip's latest patch?
>>
>> I have added this issue in PostgreSQL 10 Open Items list so that we
>> don't loose track of this issue.
>
> The result is correct with this patch.  I ran make installcheck then
> the same steps as above and the query result was correct after
> creating the index.

Thanks for confirming.

-- 
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] Problem in Parallel Bitmap Heap Scan?

[Thomas Munro]

On Sat, Mar 25, 2017 at 6:04 PM, Amit Kapila  wrote:
> On Tue, Mar 21, 2017 at 5:51 PM, Dilip Kumar  wrote:
>> On Tue, Mar 21, 2017 at 4:47 PM, Thomas Munro
>>  wrote:
>>> I noticed a failure in the inet.sql test while running the regression
>>> tests with parallelism cranked up, and can reproduce it interactively
>>> as follows.  After an spgist index is created and the plan changes to
>>> the one shown below, the query returns no rows.
>>
>> Thanks for reporting.  Seems like we are getting issues related to
>> TBM_ONE_PAGE and TBM_EMPTY.
>>
>> I think in this area we need more testing, reason these are not tested
>> properly because these are not the natural case for parallel bitmap.
>> I think in next few days I will test more such cases by forcing the
>> parallel bitmap.
>>
>
> Okay, is your testing complete?
>
>> Here is the patch to fix the issue in hand.  I have also run the
>> regress suit with force_parallel_mode=regress and all the test are
>> passing.
>>
>
> Thomas, did you get chance to verify Dilip's latest patch?
>
> I have added this issue in PostgreSQL 10 Open Items list so that we
> don't loose track of this issue.

The result is correct with this patch.  I ran make installcheck then
the same steps as above and the query result was correct after
creating the index.

-- 
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] Problem in Parallel Bitmap Heap Scan?

[Amit Kapila]

On Tue, Mar 21, 2017 at 5:51 PM, Dilip Kumar  wrote:
> On Tue, Mar 21, 2017 at 4:47 PM, Thomas Munro
>  wrote:
>> I noticed a failure in the inet.sql test while running the regression
>> tests with parallelism cranked up, and can reproduce it interactively
>> as follows.  After an spgist index is created and the plan changes to
>> the one shown below, the query returns no rows.
>
> Thanks for reporting.  Seems like we are getting issues related to
> TBM_ONE_PAGE and TBM_EMPTY.
>
> I think in this area we need more testing, reason these are not tested
> properly because these are not the natural case for parallel bitmap.
> I think in next few days I will test more such cases by forcing the
> parallel bitmap.
>

Okay, is your testing complete?

> Here is the patch to fix the issue in hand.  I have also run the
> regress suit with force_parallel_mode=regress and all the test are
> passing.
>

Thomas, did you get chance to verify Dilip's latest patch?

I have added this issue in PostgreSQL 10 Open Items list so that we
don't loose track of this issue.

-- 
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] Problem in Parallel Bitmap Heap Scan?

[Dilip Kumar]

On Wed, Mar 22, 2017 at 5:38 AM, Thomas Munro
 wrote:
> Isn't that one row short?  What happened to this one?
>
>  10.0.0.0/8 | 10::/8

Actually, In my last test I did not connect to regression database, I
have simply taken table and the few rows from inet.sql so it was only
16 rows even with seqscan.

Here are the updated results when I connect to regression database and re-test.

regression=# SELECT * FROM inet_tbl WHERE i <> '192.168.1.0/24'::cidr
ORDER BY i;
 c  |i
+--
 10.0.0.0/8 | 9.1.2.3/8
 10.0.0.0/8 | 10.1.2.3/8
 10.0.0.0/32| 10.1.2.3/8
 10.0.0.0/8 | 10.1.2.3/8
 10.1.0.0/16| 10.1.2.3/16
 10.1.2.0/24| 10.1.2.3/24
 10.1.2.3/32| 10.1.2.3
 10.0.0.0/8 | 11.1.2.3/8
 192.168.1.0/24 | 192.168.1.226/24
 192.168.1.0/24 | 192.168.1.255/24
 192.168.1.0/24 | 192.168.1.0/25
 192.168.1.0/24 | 192.168.1.255/25
 192.168.1.0/26 | 192.168.1.226
 10.0.0.0/8 | 10::/8
 :::1.2.3.4/128 | ::4.3.2.1/24
 10:23::f1/128  | 10:23::f1/64
 10:23::8000/113| 10:23::
(17 rows)

regression=# explain analyze SELECT * FROM inet_tbl WHERE i <>
'192.168.1.0/24'::cidr
ORDER BY i;
QUERY PLAN
---
 Gather Merge  (cost=16.57..16.67 rows=10 width=64) (actual
time=4.972..4.983 rows=17 loops=1)
   Workers Planned: 1
   Workers Launched: 1
   ->  Sort  (cost=16.56..16.58 rows=10 width=64) (actual
time=0.107..0.110 rows=8 loops=2)
 Sort Key: i
 Sort Method: quicksort  Memory: 26kB
 ->  Parallel Bitmap Heap Scan on inet_tbl  (cost=12.26..16.39
rows=10 width=64) (actual time=0.051..0.053 rows=8 loops=2)
   Recheck Cond: (i <> '192.168.1.0/24'::inet)
   Heap Blocks: exact=1
   ->  Bitmap Index Scan on inet_idx3  (cost=0.00..12.26
rows=17 width=0) (actual time=0.016..0.016 rows=17 loops=1)
 Index Cond: (i <> '192.168.1.0/24'::inet)
 Planning time: 0.113 ms
 Execution time: 5.691 ms
(13 rows)


-- 
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] Problem in Parallel Bitmap Heap Scan?

[Thomas Munro]

On Wed, Mar 22, 2017 at 1:21 AM, Dilip Kumar  wrote:
> postgres=# SELECT * FROM inet_tbl WHERE i <> '192.168.1.0/24'::cidr
> ORDER BY i;
>  c  |i
> +--
>  10.0.0.0/8 | 9.1.2.3/8
>  10.0.0.0/8 | 10.1.2.3/8
>  10.0.0.0/32| 10.1.2.3/8
>  10.0.0.0/8 | 10.1.2.3/8
>  10.1.0.0/16| 10.1.2.3/16
>  10.1.2.0/24| 10.1.2.3/24
>  10.1.2.3/32| 10.1.2.3
>  10.0.0.0/8 | 11.1.2.3/8
>  192.168.1.0/24 | 192.168.1.226/24
>  192.168.1.0/24 | 192.168.1.255/24
>  192.168.1.0/24 | 192.168.1.0/25
>  192.168.1.0/24 | 192.168.1.255/25
>  192.168.1.0/26 | 192.168.1.226
>  :::1.2.3.4/128 | ::4.3.2.1/24
>  10:23::f1/128  | 10:23::f1/64
>  10:23::8000/113| 10:23::
> (16 rows)

Isn't that one row short?  What happened to this one?

 10.0.0.0/8 | 10::/8

-- 
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] Problem in Parallel Bitmap Heap Scan?

[Dilip Kumar]

On Tue, Mar 21, 2017 at 4:47 PM, Thomas Munro
 wrote:
> I noticed a failure in the inet.sql test while running the regression
> tests with parallelism cranked up, and can reproduce it interactively
> as follows.  After an spgist index is created and the plan changes to
> the one shown below, the query returns no rows.

Thanks for reporting.  Seems like we are getting issues related to
TBM_ONE_PAGE and TBM_EMPTY.

I think in this area we need more testing, reason these are not tested
properly because these are not the natural case for parallel bitmap.
I think in next few days I will test more such cases by forcing the
parallel bitmap.

Here is the patch to fix the issue in hand.  I have also run the
regress suit with force_parallel_mode=regress and all the test are
passing.

Results after fix.

postgres=# explain analyze SELECT * FROM inet_tbl WHERE i <>
'192.168.1.0/24'::cidr
ORDER BY i;
QUERY PLAN
--
 Gather Merge  (cost=16.53..16.62 rows=9 width=64) (actual
time=4.467..4.478 rows=16 loops=1)
   Workers Planned: 1
   Workers Launched: 1
   ->  Sort  (cost=16.52..16.54 rows=9 width=64) (actual
time=0.090..0.093 rows=8 loops=2)
 Sort Key: i
 Sort Method: quicksort  Memory: 25kB
 ->  Parallel Bitmap Heap Scan on inet_tbl  (cost=12.26..16.37
rows=9 width=64) (actual time=0.048..0.050 rows=8 loops=2)
   Recheck Cond: (i <> '192.168.1.0/24'::inet)
   Heap Blocks: exact=1
   ->  Bitmap Index Scan on inet_idx3  (cost=0.00..12.25
rows=16 width=0) (actual time=0.016..0.016 rows=16 loops=1)
 Index Cond: (i <> '192.168.1.0/24'::inet)
 Planning time: 0.149 ms
 Execution time: 5.143 ms
(13 rows)

postgres=# SELECT * FROM inet_tbl WHERE i <> '192.168.1.0/24'::cidr
ORDER BY i;
 c  |i
+--
 10.0.0.0/8 | 9.1.2.3/8
 10.0.0.0/8 | 10.1.2.3/8
 10.0.0.0/32| 10.1.2.3/8
 10.0.0.0/8 | 10.1.2.3/8
 10.1.0.0/16| 10.1.2.3/16
 10.1.2.0/24| 10.1.2.3/24
 10.1.2.3/32| 10.1.2.3
 10.0.0.0/8 | 11.1.2.3/8
 192.168.1.0/24 | 192.168.1.226/24
 192.168.1.0/24 | 192.168.1.255/24
 192.168.1.0/24 | 192.168.1.0/25
 192.168.1.0/24 | 192.168.1.255/25
 192.168.1.0/26 | 192.168.1.226
 :::1.2.3.4/128 | ::4.3.2.1/24
 10:23::f1/128  | 10:23::f1/64
 10:23::8000/113| 10:23::
(16 rows)


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


fix_parallel_bitmap_handling_onepage.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] Problem in Parallel Bitmap Heap Scan?

[Thomas Munro]

Hi,

I noticed a failure in the inet.sql test while running the regression
tests with parallelism cranked up, and can reproduce it interactively
as follows.  After an spgist index is created and the plan changes to
the one shown below, the query returns no rows.

regression=# set force_parallel_mode = regress;
SET
regression=# set max_parallel_workers_per_gather = 2;
SET
regression=# set parallel_tuple_cost = 0;
SET
regression=# set parallel_setup_cost = 0;
SET
regression=# set min_parallel_table_scan_size = 0;
SET
regression=# set min_parallel_index_scan_size = 0;
SET
regression=# set enable_seqscan = off;
SET
regression=# SELECT * FROM inet_tbl WHERE i <> '192.168.1.0/24'::cidr
ORDER BY i;

c  |i
+--
 10.0.0.0/8 | 9.1.2.3/8
 10.0.0.0/8 | 10.1.2.3/8
 10.0.0.0/32| 10.1.2.3/8
 10.0.0.0/8 | 10.1.2.3/8
 10.1.0.0/16| 10.1.2.3/16
 10.1.2.0/24| 10.1.2.3/24
 10.1.2.3/32| 10.1.2.3
 10.0.0.0/8 | 11.1.2.3/8
 192.168.1.0/24 | 192.168.1.226/24
 192.168.1.0/24 | 192.168.1.255/24
 192.168.1.0/24 | 192.168.1.0/25
 192.168.1.0/24 | 192.168.1.255/25
 192.168.1.0/26 | 192.168.1.226
 10.0.0.0/8 | 10::/8
 :::1.2.3.4/128 | ::4.3.2.1/24
 10:23::f1/128  | 10:23::f1/64
 10:23::8000/113| 10:23::
(17 rows)

regression=# CREATE INDEX inet_idx3 ON inet_tbl using spgist (i);
CREATE INDEX
regression=# SELECT * FROM inet_tbl WHERE i <> '192.168.1.0/24'::cidr
ORDER BY i;
 c | i
---+---
(0 rows)

regression=# explain SELECT * FROM inet_tbl WHERE i <>
'192.168.1.0/24'::cidr ORDER BY i;
   QUERY PLAN
-
 Gather Merge  (cost=16.57..16.67 rows=10 width=64)
   Workers Planned: 1
   ->  Sort  (cost=16.56..16.58 rows=10 width=64)
 Sort Key: i
 ->  Parallel Bitmap Heap Scan on inet_tbl  (cost=12.26..16.39
rows=10 width=64)
   Recheck Cond: (i <> '192.168.1.0/24'::inet)
   ->  Bitmap Index Scan on inet_idx3  (cost=0.00..12.26
rows=17 width=0)
 Index Cond: (i <> '192.168.1.0/24'::inet)
(8 rows)


-- 
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] Problem in PostgresSQL Configuration with YII 1 & Wordpress

[Craig Ringer]

On 24 July 2016 at 02:20, Jyoti Sharma  wrote:

> Hi Team,
>
> Currently we have a project running with MySQL with YII & Wordpress, Now i
> want to change the Database from MYSQL to PostgreSQL.
>

Hi.

The pgsql-hackers mailing list is for development of PostgreSQL its self.
General questions are better suited to the pgsql-general mailing list or to
Stack Overflow.

Conversion from MySQL to PostgreSQL is a common question and there's lots
of info out there, though you might like the answers since there are't any
automagic tools to make all existing queries "just work". As for junk
characters, look at the "bytea" data type. For more information please post
at one of the more appropriate locations given above; I will not reply to
responses to this mail on pgsql-hackers.

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

[HACKERS] Problem in PostgresSQL Configuration with YII 1 & Wordpress

[Jyoti Sharma]

Hi Team,

Currently we have a project running with MySQL with YII & Wordpress, Now i
want to change the Database from MYSQL to PostgreSQL.

Please put your inline comments below.
*[JS:] *Is there any way we can convert MYSQL queries to PostreSQL Queries?

*Your Comment : ?*


*[JS:] *Is there any way we can insert serialize data with junked character
in PostgreSQL?


*Your Comment : ?*


*Can we have a meeting with any of your technical spokesperson.*

*Our Frameworks & software version we are using.*
1. YII 1.1.1.4
2. PostgreSQL 9.3
3. Wordpress
4. MYSQL
5. OS : Ubuntu


--
Thanks
Jyoti Sharma

-- 
 



*This e-mail and all attachments are intended solely for use by the 
intended recipient and may contain confidential / proprietary information 
of KiwiTech, LLC, subject to important disclaimers and conditions including 
restrictions on the use, disclosure, transfer or export of such 
information.* *If you have received this message in error or are not the 
named recipient(s), please immediately notify the sender at the telephone 
number stated above or by reply e-mail and delete this e-mail from your 
computer*

Re: [HACKERS] Problem with dumping bloom extension

[Stephen Frost]

* Noah Misch (n...@leadboat.com) wrote:
> Yep, pretty much that.  CLOSE_WAIT is for performance defects, race
> conditions, and other defects where a successful fix is difficult to verify
> beyond reasonable doubt.  Other things can move directly to "resolved".  I
> don't mind if practice diverges from that intent, and I don't really process
> the two sections differently.

Ok, thanks for the clarification.  I've updated the entries which I had
accordingly.

Thanks again!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Problem with dumping bloom extension

[Noah Misch]

On Tue, Jun 07, 2016 at 03:23:46PM -0400, Robert Haas wrote:
> On Tue, Jun 7, 2016 at 2:40 PM, Peter Eisentraut
>  wrote:
> > On 6/7/16 11:16 AM, Stephen Frost wrote:
> >>
> >> Moved to CLOSE_WAIT.
> >
> > Could you add an explanation on the wiki page about what this section means?
> 
> Noah created that section.  My interpretation is that it's supposed to
> be for things we think are fixed, but maybe there's a chance they
> aren't - like a performance problem that we've patched but not
> received confirmation from the original reporter that the patch fixes
> it for them.  I'm inclined to think that most issues should just
> become "resolved" right away.

Yep, pretty much that.  CLOSE_WAIT is for performance defects, race
conditions, and other defects where a successful fix is difficult to verify
beyond reasonable doubt.  Other things can move directly to "resolved".  I
don't mind if practice diverges from that intent, and I don't really process
the two sections differently.


-- 
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] Problem with dumping bloom extension

[Stephen Frost]

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 6/7/16 11:16 AM, Stephen Frost wrote:
> >Moved to CLOSE_WAIT.
> 
> Could you add an explanation on the wiki page about what this section means?

I understood it to simply be a step on the way to being resolved- that
is, everything goes through CLOSE_WAIT for some period of time and then
is moved to resolved when it's clear that the consensus is that it's
closed.

That doesn't appear to be the consensus on the meaning though, and I
didn't add it, so I'm not the one to update the wiki page to explain it.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Problem with dumping bloom extension

[Robert Haas]

On Tue, Jun 7, 2016 at 2:40 PM, Peter Eisentraut
 wrote:
> On 6/7/16 11:16 AM, Stephen Frost wrote:
>>
>> Moved to CLOSE_WAIT.
>
> Could you add an explanation on the wiki page about what this section means?

Noah created that section.  My interpretation is that it's supposed to
be for things we think are fixed, but maybe there's a chance they
aren't - like a performance problem that we've patched but not
received confirmation from the original reporter that the patch fixes
it for them.  I'm inclined to think that most issues should just
become "resolved" right away.

-- 
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] Problem with dumping bloom extension

[Peter Eisentraut]

On 6/7/16 11:16 AM, Stephen Frost wrote:

Moved to CLOSE_WAIT.


Could you add an explanation on the wiki page about what this section means?

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

Re: [HACKERS] Problem with dumping bloom extension

[Stephen Frost]

* Noah Misch (n...@leadboat.com) wrote:
> The above-described topic is currently a PostgreSQL 9.6 open item.  Stephen,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> 9.6 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within 72 hours of this
> message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
> efforts toward speedy resolution.  Thanks.

Addressed with 562f06f.

Moved to CLOSE_WAIT.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Problem with dumping bloom extension

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> Stephen, are you working on a patch or should I get one out of my
> pocket? That's something we should get fixed quickly. We need as well
> to be careful with the support for COMMENT with access methods, the
> current consensus on the matter is that it will be soon committed.

I'll fix it today.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Problem with dumping bloom extension

[Michael Paquier]

On Tue, Jun 7, 2016 at 8:10 PM, Robert Haas  wrote:
> On Mon, Jun 6, 2016 at 5:55 PM, Michael Paquier
>  wrote:
>>> It seems important to get this fixed.  I added it to the open items list.
>>
>> I added already it as " Access methods created with extensions are
>> dumped individually ". That's not specific to bloom.
>
> Oh, sorry, I didn't spot that.

Never mind. I refreshed the wiki again after that, and kept your entry
which uses directly $subject.
-- 
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] Problem with dumping bloom extension

[Robert Haas]

On Mon, Jun 6, 2016 at 5:55 PM, Michael Paquier
 wrote:
>> It seems important to get this fixed.  I added it to the open items list.
>
> I added already it as " Access methods created with extensions are
> dumped individually ". That's not specific to bloom.

Oh, sorry, I didn't spot that.

-- 
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] Problem with dumping bloom extension

[Noah Misch]

On Fri, Jun 03, 2016 at 12:31:44PM -0400, Stephen Frost wrote:
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
> > On Fri, Jun 3, 2016 at 8:57 PM, Thom Brown  wrote:
> > > If a database with the bloom extension installed is dumped and restored,
> > > there's an error with the access method creation:
> > >
> > > createdb bloomtest
> > > psql -c 'CREATE EXTENSION bloom;' bloomtest
> > > pg_dump -d bloomtest > ~/tmp/bloom.sql
> > > createdb bloomtest2
> > > psql -d bloomtest2 -f ~/tmp/bloom.sql
> > >
> > > The output of the last command produces:
> > >
> > > "psql:/home/thom/tmp/bloom.sql:48: ERROR:  access method "bloom" already
> > > exists"
> > >
> > > So pg_dump shouldn't be dumping this access method as it's part of the
> > > extension.
> > 
> > Stephen, something is smelling wrong in checkExtensionMembership()
> > since 5d58999, an access method does not have ACLs and I would have
> > expected that when this routine is invoked in
> > selectDumpableAccessMethod() the object is not selected as dumpable.
> 
> Yeah, I saw this also and was going to look into it.

[Action required within 72 hours.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item.  Stephen,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within 72 hours of this
message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
efforts toward speedy resolution.  Thanks.

[1] 
http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.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] Problem with dumping bloom extension

[Michael Paquier]

On Tue, Jun 7, 2016 at 6:55 AM, Michael Paquier
 wrote:
> On Tue, Jun 7, 2016 at 12:01 AM, Robert Haas  wrote:
>> On Fri, Jun 3, 2016 at 12:31 PM, Stephen Frost  wrote:
 Stephen, something is smelling wrong in checkExtensionMembership()
 since 5d58999, an access method does not have ACLs and I would have
 expected that when this routine is invoked in
 selectDumpableAccessMethod() the object is not selected as dumpable.
>>>
>>> Yeah, I saw this also and was going to look into it.
>>>
>>> I suspect the issue may actually be that dumpAccessMethod() wasn't ever
>>> updated to have the appropriate conditionals for each of the components
>>> of the object.
>>>
>>> Specifically, there should be if statements along the lines of:
>>>
>>> if (aminfo->dobj.dump & DUMP_COMPONENT_DEFINITION)
>>> ArchiveEntry ...
>>>
>>> if (aminfo->dobj.dump & DUMP_COMPONENT_COMMENT)
>>> dumpComment()
>>>
>>> towards the end of dumpAccessMethod().
>>>
>>> I'm not 100% sure that addresses this, but it definitely needs to be
>>> fixed also.  I'll take care of it in the next few days.
>>>
>>> I'll also look more directly into what's going on here this weekend when
>>> I've got a bit more time to do so.
>>
>> It seems important to get this fixed.  I added it to the open items list.

Stephen, are you working on a patch or should I get one out of my
pocket? That's something we should get fixed quickly. We need as well
to be careful with the support for COMMENT with access methods, the
current consensus on the matter is that it will be soon committed.
-- 
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] Problem with dumping bloom extension

[Michael Paquier]

On Tue, Jun 7, 2016 at 12:01 AM, Robert Haas  wrote:
> On Fri, Jun 3, 2016 at 12:31 PM, Stephen Frost  wrote:
>>> Stephen, something is smelling wrong in checkExtensionMembership()
>>> since 5d58999, an access method does not have ACLs and I would have
>>> expected that when this routine is invoked in
>>> selectDumpableAccessMethod() the object is not selected as dumpable.
>>
>> Yeah, I saw this also and was going to look into it.
>>
>> I suspect the issue may actually be that dumpAccessMethod() wasn't ever
>> updated to have the appropriate conditionals for each of the components
>> of the object.
>>
>> Specifically, there should be if statements along the lines of:
>>
>> if (aminfo->dobj.dump & DUMP_COMPONENT_DEFINITION)
>> ArchiveEntry ...
>>
>> if (aminfo->dobj.dump & DUMP_COMPONENT_COMMENT)
>> dumpComment()
>>
>> towards the end of dumpAccessMethod().
>>
>> I'm not 100% sure that addresses this, but it definitely needs to be
>> fixed also.  I'll take care of it in the next few days.
>>
>> I'll also look more directly into what's going on here this weekend when
>> I've got a bit more time to do so.
>
> It seems important to get this fixed.  I added it to the open items list.

I added already it as " Access methods created with extensions are
dumped individually ". That's not specific to bloom.
-- 
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] Problem with dumping bloom extension

[Robert Haas]

On Fri, Jun 3, 2016 at 12:31 PM, Stephen Frost  wrote:
>> Stephen, something is smelling wrong in checkExtensionMembership()
>> since 5d58999, an access method does not have ACLs and I would have
>> expected that when this routine is invoked in
>> selectDumpableAccessMethod() the object is not selected as dumpable.
>
> Yeah, I saw this also and was going to look into it.
>
> I suspect the issue may actually be that dumpAccessMethod() wasn't ever
> updated to have the appropriate conditionals for each of the components
> of the object.
>
> Specifically, there should be if statements along the lines of:
>
> if (aminfo->dobj.dump & DUMP_COMPONENT_DEFINITION)
> ArchiveEntry ...
>
> if (aminfo->dobj.dump & DUMP_COMPONENT_COMMENT)
> dumpComment()
>
> towards the end of dumpAccessMethod().
>
> I'm not 100% sure that addresses this, but it definitely needs to be
> fixed also.  I'll take care of it in the next few days.
>
> I'll also look more directly into what's going on here this weekend when
> I've got a bit more time to do so.

It seems important to get this fixed.  I added it to the open items list.

-- 
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] Problem with dumping bloom extension

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Fri, Jun 3, 2016 at 8:57 PM, Thom Brown  wrote:
> > If a database with the bloom extension installed is dumped and restored,
> > there's an error with the access method creation:
> >
> > createdb bloomtest
> > psql -c 'CREATE EXTENSION bloom;' bloomtest
> > pg_dump -d bloomtest > ~/tmp/bloom.sql
> > createdb bloomtest2
> > psql -d bloomtest2 -f ~/tmp/bloom.sql
> >
> > The output of the last command produces:
> >
> > "psql:/home/thom/tmp/bloom.sql:48: ERROR:  access method "bloom" already
> > exists"
> >
> > So pg_dump shouldn't be dumping this access method as it's part of the
> > extension.
> 
> Stephen, something is smelling wrong in checkExtensionMembership()
> since 5d58999, an access method does not have ACLs and I would have
> expected that when this routine is invoked in
> selectDumpableAccessMethod() the object is not selected as dumpable.

Yeah, I saw this also and was going to look into it.

I suspect the issue may actually be that dumpAccessMethod() wasn't ever
updated to have the appropriate conditionals for each of the components
of the object.

Specifically, there should be if statements along the lines of:

if (aminfo->dobj.dump & DUMP_COMPONENT_DEFINITION)
ArchiveEntry ...

if (aminfo->dobj.dump & DUMP_COMPONENT_COMMENT)
dumpComment()

towards the end of dumpAccessMethod().

I'm not 100% sure that addresses this, but it definitely needs to be
fixed also.  I'll take care of it in the next few days.

I'll also look more directly into what's going on here this weekend when
I've got a bit more time to do so.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Problem with dumping bloom extension

[Michael Paquier]

On Fri, Jun 3, 2016 at 8:57 PM, Thom Brown  wrote:
> If a database with the bloom extension installed is dumped and restored,
> there's an error with the access method creation:
>
> createdb bloomtest
> psql -c 'CREATE EXTENSION bloom;' bloomtest
> pg_dump -d bloomtest > ~/tmp/bloom.sql
> createdb bloomtest2
> psql -d bloomtest2 -f ~/tmp/bloom.sql
>
> The output of the last command produces:
>
> "psql:/home/thom/tmp/bloom.sql:48: ERROR:  access method "bloom" already
> exists"
>
> So pg_dump shouldn't be dumping this access method as it's part of the
> extension.

Stephen, something is smelling wrong in checkExtensionMembership()
since 5d58999, an access method does not have ACLs and I would have
expected that when this routine is invoked in
selectDumpableAccessMethod() the object is not selected as dumpable.
-- 
Michael


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

[HACKERS] Problem with dumping bloom extension

[Thom Brown]

Hi,

If a database with the bloom extension installed is dumped and restored,
there's an error with the access method creation:

createdb bloomtest
psql -c 'CREATE EXTENSION bloom;' bloomtest
pg_dump -d bloomtest > ~/tmp/bloom.sql
createdb bloomtest2
psql -d bloomtest2 -f ~/tmp/bloom.sql

The output of the last command produces:

"psql:/home/thom/tmp/bloom.sql:48: ERROR:  access method "bloom" already
exists"

So pg_dump shouldn't be dumping this access method as it's part of the
extension.

Regards

Thom

Re: [HACKERS] problem with precendence order in JSONB merge operator

[David G. Johnston]

Please don't top-post.

On Tuesday, March 22, 2016, Peter Krauss  wrote:

> Subjective notes to contextualize (try to explain on bad-English) my
> "precedence order" and JSONB visions:
>
> JSON datatype is perfect as workaround, and for many simple and less
> exigent applications.
> JSONB is the  "first class" datatype for user community, we expected years
> (!) for it ... Need some "first class" and friendly behaviour.
>
> In this context JSONB is not "any other" datatype, it is the bridge
> between relational data and flexible data...
> It is the Holy Grail and the Rosetta Stone :-)
>
> I think JSONB operators need some more attention, in semantic and
> usability contexts.   If you want to add  some friendliness and
> orthogonality in JSONB operators, will be natural to see -> operator as a
> kind of object-oriented *path* operator...
> By other hand, of course, you can do the simplest to implement JSONB...
> But you do a lot
>  (!), it
> was not easy to arrive here, and need only a little bit more to  reach
> perfection ;-)
>
>
You are welcome to supply a patch for this particular "little bit" - but I
suspect you will find it quite disruptive to backward compatibility and
general system internals if you attempt to do so.  But maybe not.

Any change you make in this area will effect every usage of that operator
whether part of core or end-user defined.  We have baggage that limits
our ability to be perfect.

So while I'll agree with your premise I don't see (really, don't care to
look for) a way to make your desire a reality.  But you and smarter people
than I are welcome to dive into the code and see if you can come up with
something that works.

David J.

Re: [HACKERS] problem with precendence order in JSONB merge operator

[Peter Krauss]

Subjective notes to contextualize (try to explain on bad-English) my
"precedence order" and JSONB visions:

JSON datatype is perfect as workaround, and for many simple and less
exigent applications.
JSONB is the  "first class" datatype for user community, we expected years
(!) for it ... Need some "first class" and friendly behaviour.

In this context JSONB is not "any other" datatype, it is the bridge between
relational data and flexible data...
It is the Holy Grail and the Rosetta Stone :-)

I think JSONB operators need some more attention, in semantic and usability
contexts.   If you want to add  some friendliness and orthogonality in
JSONB operators, will be natural to see -> operator as a kind of
object-oriented *path* operator...
By other hand, of course, you can do the simplest to implement JSONB... But
you do a lot 
(!), it was not easy to arrive here, and need only a little bit more
to  reach perfection ;-)



2016-03-22 18:42 GMT-03:00 David G. Johnston :

> On Tue, Mar 22, 2016 at 1:52 PM, Peter Krauss  wrote:
>
>> Seems that parser not using precedence ideal order, and that casting
>> obligation losts performance.
>>
>> The first problem is self-evident in this example:
>>
>> SELECT '{"x":1}'::jsonb || (('{"A":{"y":2}}'::jsonb)->'A')
>>   -- it is ok, expected result with (x,y)
>> SELECT '{"x":1}'::jsonb || '{"A":{"y":2}}'::jsonb)->'A'
>>   -- non-expected result (y).
>>
>> Higher precedence  most
>> be for -> operator, that is like an object-oriented *path* operator,
>> always higher than algebric ones.
>>
> ​There is presently no formal concept of "path operator" in PostgreSQL.
>  "->" is a user-defined operator, as is "||"​ and thus have equal
> precedence and left associativity.
>
> http://www.postgresql.org/docs/current/static/sql-syntax-lexical.html
>
> Regardless, "||" is not an "algebric" [sic] operator...I'm curious what
> source you are using to back your claim of operator precedence between
> different so-called "operator types".
>
> Its highly undesirable to make changes to operator precedence.
>
> Operators are simply symbols to the parser - there is no context involved
> that would allow making their precedence dynamic.  So all PostgreSQL sees
> is "||", not a "JSONB merge operator".
>
> Other problem is using this operation as SQL function
>>
>>   CREATE FUNCTION term_lib.junpack(jsonb,text) RETURNS JSONB AS $f$
>> SELECT ($1-$2)::JSONB || ($1->>$2)::JSONB;
>>   $f$ LANGUAGE SQL IMMUTABLE;
>>
>> without casting produce error. Perhaps will be "more friendly" without
>> cast obligation,
>>
>> and it is a performance problem, the abusive use of castings losts
>> performance.
>>
> I cannot make this work...
>
> version
> PostgreSQL 9.5.1 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
> 4.8.2-19ubuntu1) 4.8.2, 64-bit
>
> SELECT ('{"a":1,"b":2}'::jsonb - 'b'::text)::jsonb ||
> ('{"a":1,"b":2}'::jsonb #> 'b'::text)::jsonb
>
> > ​SQL Error: ERROR: invalid concatenation of jsonb objects
> ​
> This seems like user error but without a self-contained test case
> exercising the query (the use of a function in this context should be
> immaterial) I'm finding it hard to explain why.  My simple case returns a
> non-object with rightly cannot be appended to an object.
>
> In isolatoin you can avoid casting the RHS of the || operator by using the
> "#>(jsonb,text[])" operator
>
> SELECT pg_typeof('{"a":1,"b":{"c":2}}'::jsonb #> array['b']::text[])
> --jsonb
>
> JSON, IME, still needs some fleshing out.  Efficient usage might require
> additional features but for now one needs to get very familiar with all the
> various operator variants that allow the user to choose whether to return
> json or text and to pick the correct one for their needs.
>
> ​David J.
> ​
>
>

Re: [HACKERS] problem with precendence order in JSONB merge operator

[David G. Johnston]

On Tue, Mar 22, 2016 at 1:52 PM, Peter Krauss  wrote:

> Seems that parser not using precedence ideal order, and that casting
> obligation losts performance.
>
> The first problem is self-evident in this example:
>
> SELECT '{"x":1}'::jsonb || (('{"A":{"y":2}}'::jsonb)->'A')
>   -- it is ok, expected result with (x,y)
> SELECT '{"x":1}'::jsonb || '{"A":{"y":2}}'::jsonb)->'A'
>   -- non-expected result (y).
>
> Higher precedence  most
> be for -> operator, that is like an object-oriented *path* operator,
> always higher than algebric ones.
>
​There is presently no formal concept of "path operator" in PostgreSQL.
 "->" is a user-defined operator, as is "||"​ and thus have equal
precedence and left associativity.

http://www.postgresql.org/docs/current/static/sql-syntax-lexical.html

Regardless, "||" is not an "algebric" [sic] operator...I'm curious what
source you are using to back your claim of operator precedence between
different so-called "operator types".

Its highly undesirable to make changes to operator precedence.

Operators are simply symbols to the parser - there is no context involved
that would allow making their precedence dynamic.  So all PostgreSQL sees
is "||", not a "JSONB merge operator".

Other problem is using this operation as SQL function
>
>   CREATE FUNCTION term_lib.junpack(jsonb,text) RETURNS JSONB AS $f$
> SELECT ($1-$2)::JSONB || ($1->>$2)::JSONB;
>   $f$ LANGUAGE SQL IMMUTABLE;
>
> without casting produce error. Perhaps will be "more friendly" without
> cast obligation,
>
> and it is a performance problem, the abusive use of castings losts
> performance.
>
I cannot make this work...

version
PostgreSQL 9.5.1 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
4.8.2-19ubuntu1) 4.8.2, 64-bit

SELECT ('{"a":1,"b":2}'::jsonb - 'b'::text)::jsonb ||
('{"a":1,"b":2}'::jsonb #> 'b'::text)::jsonb

> ​SQL Error: ERROR: invalid concatenation of jsonb objects
​
This seems like user error but without a self-contained test case
exercising the query (the use of a function in this context should be
immaterial) I'm finding it hard to explain why.  My simple case returns a
non-object with rightly cannot be appended to an object.

In isolatoin you can avoid casting the RHS of the || operator by using the
"#>(jsonb,text[])" operator

SELECT pg_typeof('{"a":1,"b":{"c":2}}'::jsonb #> array['b']::text[]) --jsonb

JSON, IME, still needs some fleshing out.  Efficient usage might require
additional features but for now one needs to get very familiar with all the
various operator variants that allow the user to choose whether to return
json or text and to pick the correct one for their needs.

​David J.
​

[HACKERS] problem with precendence order in JSONB merge operator

[Peter Krauss]

Seems that parser not using precedence ideal order, and that casting
obligation losts performance.

The first problem is self-evident in this example:

SELECT '{"x":1}'::jsonb || (('{"A":{"y":2}}'::jsonb)->'A')
  -- it is ok, expected result with (x,y)
SELECT '{"x":1}'::jsonb || '{"A":{"y":2}}'::jsonb)->'A'
  -- non-expected result (y).

Higher precedence  most
be for -> operator, that is like an object-oriented *path* operator, always
higher than algebric ones.


Other problem is using this operation as SQL function,

  CREATE FUNCTION term_lib.junpack(jsonb,text) RETURNS JSONB AS $f$
SELECT ($1-$2)::JSONB || ($1->>$2)::JSONB;
  $f$ LANGUAGE SQL IMMUTABLE;

without casting produce error. Perhaps will be "more friendly" without cast
obligation,

and it is a performance problem, the abusive use of castings losts
performance.

Re: [HACKERS] problem with msvc linker - cannot build orafce

[Pavel Stehule]

2015-11-23 21:14 GMT+01:00 Tom Lane :

> Pavel Stehule  writes:
> > I am trying to build Orafce and I have problem due access to exported
> > variable session_timezone.
> > Any idea what can be broken?
>
> Lack of PGDLLIMPORT on the extern declaration, no doubt.
>
> The fact that we've not heard this before implies that either nobody has
> ever tried to use orafce on Windows, or it only very recently grew a
> dependency on session_timezone.
>

yes, last "official" win build on pgfoundry is for 9.0.

Regards

Pavel


>
> regards, tom lane
>

Re: [HACKERS] problem with msvc linker - cannot build orafce

[Pavel Stehule]

2015-11-24 11:33 GMT+01:00 Kisung Kim :

> 2015-11-24 8:12 GMT+09:00 Chapman Flack :
>
>> On 11/23/15 15:14, Tom Lane wrote:
>> > Lack of PGDLLIMPORT on the extern declaration, no doubt.
>> >
>> > The fact that we've not heard this before implies that either nobody has
>> > ever tried to use orafce on Windows, or it only very recently grew a
>> > dependency on session_timezone.
>>
>>
> Actually, we encountered the situation before couple of months.
> A client wanted to use orafce on Windows and the same build problem
> occurred.
> We performed a workaround to edit the PG source to export unresolved
> symbols,
> which I think of not a good solution.
>

I found a workaround for session_timezone so master is compileable on msvc
again without patching PG source

https://github.com/orafce/orafce/commit/9bda5074d44eefebbd002b4720d5833e612428af

but can be nice to solve this issue - there can be some interesting
variables and not for all the workaround is possible

Regards

Pavel



>
> 2015-11-24 8:12 GMT+09:00 Chapman Flack :
>
>> Has anyone got the stomach to try such a thing and see what happens?
>> I don't have MSVC here.
>>
>> -Chap
>
>
> We have the environment to test your ideas.
> Can you explain your ideas with more detail?
>
>
>
>
>
>
> (C)Bitnine, Kisung Kim, Ph.D
> https://sites.google.com/site/kisungresearch/
> E-mail : ks...@bitnine.co.kr
> Office phone : 070-4800-3321
> Mobile phone : 010-7136-0834
> Fax : 02-568-1332
>
>

Re: [HACKERS] problem with msvc linker - cannot build orafce

[Pavel Stehule]

Dne 24. 11. 2015 15:44 napsal uživatel "Chapman Flack" <
c...@anastigmatix.net>:
>
> On 11/24/2015 05:33 AM, Kisung Kim wrote:
> > 2015-11-24 8:12 GMT+09:00 Chapman Flack :
> >> On 11/23/15 15:14, Tom Lane wrote:
> >>> Lack of PGDLLIMPORT on the extern declaration, no doubt.
> >>
> > Actually, we encountered the situation before couple of months.
> > A client wanted to use orafce on Windows and the same build problem
> > occurred.
> > We performed a workaround to edit the PG source to export unresolved
> > symbols,
> > which I think of not a good solution.
>
> >> Has anyone got the stomach to try such a thing and see what happens?
> >> I don't have MSVC here.
> >
> > We have the environment to test your ideas.
> > Can you explain your ideas with more detail?
>
> Well, the main idea is just this:  *IF* it is sufficient to declare
> a variable PGDLLIMPORT only in the code that is importing it (the
> big IF because I don't know whether that is true, but something I
> saw in that long earlier thread seemed to suggest it) ...
>
> Then ... the chief problem that needs to be solved is only that
> MSVC won't allow you to redeclare something with PGDLLIMPORT if
> it is also declared without PGDLLIMPORT in a .h file that you
> include. In other words, you can't simply:
>
> #include 
> extern PGDLLIMPORT pg_tz session_timezone; /* the right way now */
>
> because it was already declared the wrong way in pgtime.h.
>
> So one idea is just this:
>
> #define session_timezone decoy_session_timezone;
> #include 
> #undef decoy_session_timezone;
>
> extern PGDLLIMPORT pg_tz session_timezone; /* the right way now */
>
> which is not a multiple declaration of the same thing, because
> what got declared the wrong way in pgtime.h is now some other thing
> named decoy_session_timezone.  You might need to supply a thing by
> that name, to avoid a linker complaint:
>
> pg_tz decoy_session_timezone; /* never used */
>
> IF the original premise is true, then this technique ought to be
> usable in most cases. It would, however, break in cases where the
> .h file declares macros or inline functions that refer to the
> symbol, because they would all end up referring to the decoy.
>
> My other idea, especially if there were several symbols needing
> to be treated this way, would be to do it all in one dedicated
> .c file, so any of the possible problems with #defining away parts
> of an .h file would be contained in one place, and that file could
> have a simple getter function:
>
> pg_tz getSessionTimezone() { return session_timezone; }
>
> which would be used in the rest of the code instead of referring
> to the global directly. (In that case, of course, the same getter
> function would have to be provided in the non-MSVC case too.)
> A setter function could also be made, if the code needs it.
>

I'll do these checks tomorrow.

Thank you very much, msvc is big unknown for me

regards

Pavel

Re: [HACKERS] problem with msvc linker - cannot build orafce

[Craig Ringer]

On 24 November 2015 at 07:12, Chapman Flack  wrote:

>
> What I (think I) took away from it was:
>
> 1.  Un-PGDLLIMPORTed references to global *functions* work ok.
> Maybe they are thunked and a little less efficient, but they work.
>
> 2.  Un-PGDLLIMPORTed references to global *variables*, not so much.
> They used to silently link (at least on some buildfarm critters)
> but hold bogus data (maybe a thunk, taken as data?).
>
> 3.  The one concrete *action* taken in the course of that thread was to
> tweak the build process to make sure such cases at least *fail*
> because that's better than silent bogosity.
>

Correct on all points.

The question that interests me most right now: how, if at all, can the
> extension author/maintainer work around this issue when it crops up?
>

AFAIK the only way to do it is to use the .pdb files to find the address of
the variable's storage, then create a pointer-to-var that way, after
adjusting for the DLL/EXE's base load address - which on some Windows
versions is randomized.

You might be able to do it with dbghelp.dll . I haven't looked into it
properly.


> Obviously, the Right Thing To Do is report it and get the PGDLLIMPORT
> added here, but still for years to come the extension will have to cope
> with being built against PG distributions that lack it.


It's safe to add in a point-release because nobody could've been using it
before, so all you have to do is require a certain minimum point release.
We all know how easy it is to get users to apply point releases ;)

If we annotated extern functions too, we could build on UNIX with
-fvisibility=hidden and get immediate linker errors on *nix too. As far as
I can tell gcc doesn't have the option to control default visibility
separately for functions and data exports. We do that on Windows by
generating a .DEF file, since Windows doesn't support it directly either.
Doing that on *nix would require using readelf then
using --retain-symbols-file at link time. Imagine how popular that'd be.

So currently we rely on the buildfarm complaining if things are broken on
Windows, but of course that only works for core and in-tree extensions.
(Who cares about anything else, right?).

See https://gcc.gnu.org/wiki/Visibility for details on visibility.

Now, I thought I spotted, somewhere in that long thread, the hint of an
> idea that the magic works as long as the *extension* has the variable
> declared PGDLLIMPORT, even if it wasn't declared that way when the PG
> executable itself was built. Anybody else remember that, or did I
> imagine it?
>

I haven't tested, but I believe you can declare it with an extra level of
pointer indirection, without __declspec(dllimport), and chase the pointer.

As I understand it (and I'm far from an expert here) the symbol in the PE
symbol table points to the address of a pointer to the data. MSVC doesn't
know if your "extern" references a variable in the same module (in which
case you don't have that indirection) or will be resolved by dynamic
linking to point to a pointer to the data. I find this bizarre, given that
ELF "just does it"... but PE is pretty weird and a fair bit older, a
hacked-up variant of COFF. Anyway, __declspec(dllimport) tells MSVC "I'm
going to be linking this from an external DLL, do the pointer chasing for
me please".

See https://msdn.microsoft.com/en-us/library/aa271769(v=vs.60).aspx "Using
__declspec(dllexport) and __declspec(dllimport) on Data".

I *think* it's safe to do this even if the var is declared
__declspec(dllexport). The dllexport declaration makes sure the export is
present without the use of a .DEF file to force it. You can
__declspec(dllimport) whether it was __declspec(dllexported)'ed or exported
via a .DEF file.

We auto-export functions in our DEF files, but not variables.


> You *might* get away with creating a separate C file (how about
> chamberofhorrors.c?) that, rather revoltingly, *doesn't* include the
> proper PostgreSQL .h files, only duplicates the necessary declarations
> with PGDLLIMPORT added, and exports some getter/setter methods
> to the rest of the extension code.  (I like the idea of one
> chamberofhorrors
> better than scattering such rubbish all over the project.)
>

I don't think that's necessary, per above. You just have to access the vars
via pointer indirection always, so long as *any* Pg version you support has
ever lacked dllexport or DEF entry, so you can't dllimport the var.

You could enable direct dllimport if PG_VERSION_NUM shows you're on a new
enough version, but you'd need to use conditionally compiled inlines or
something to switch between the methods of accessing it, so there's not
much point. You just declare

extern int* log_min_messages_p;

... and use that, probably also #define'ing log_min_messages away after
including the Pg headers so that you can't reference it accidentally.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 

Re: [HACKERS] problem with msvc linker - cannot build orafce

[Craig Ringer]

>
>
> I don't think that's necessary, per above. You just have to access the
> vars via pointer indirection always, so long as *any* Pg version you
> support has ever lacked dllexport or DEF entry, so you can't dllimport the
> var.
>
> You could enable direct dllimport if PG_VERSION_NUM shows you're on a new
> enough version, but you'd need to use conditionally compiled inlines or
> something to switch between the methods of accessing it, so there's not
> much point. You just declare
>
> extern int* log_min_messages_p;
>
> ... and use that, probably also #define'ing log_min_messages away after
> including the Pg headers so that you can't reference it accidentally.
>


Actually, if __declspec(dllexport) or a .DEF entry was added in, say,
9.4.5, you could probably just:

#if PG_VERSION_NUM < 90405
extern int* log_min_messages_p;
#define log_min_messages (*log_min_messages_p)
#endif

after including all PostgreSQL headers. It won't work for inline functions
defined in PostgreSQL headers, but should otherwise be OK I think.

(again, untested)

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

Re: [HACKERS] problem with msvc linker - cannot build orafce

[Tom Lane]

Craig Ringer  writes:
> Actually, if __declspec(dllexport) or a .DEF entry was added in, say,
> 9.4.5, you could probably just:

> #if PG_VERSION_NUM < 90405
> extern int* log_min_messages_p;
> #define log_min_messages (*log_min_messages_p)
> #endif

> after including all PostgreSQL headers. It won't work for inline functions
> defined in PostgreSQL headers, but should otherwise be OK I think.

Some of these workarounds look like they would break if we add the missing
PGDLLIMPORT in future releases.  That would be nasty :-(.  Am I misreading
it?

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] problem with msvc linker - cannot build orafce

[Craig Ringer]

On 25 November 2015 at 13:36, Tom Lane  wrote:

> Craig Ringer  writes:
> > Actually, if __declspec(dllexport) or a .DEF entry was added in, say,
> > 9.4.5, you could probably just:
>
> > #if PG_VERSION_NUM < 90405
> > extern int* log_min_messages_p;
> > #define log_min_messages (*log_min_messages_p)
> > #endif
>
> > after including all PostgreSQL headers. It won't work for inline
> functions
> > defined in PostgreSQL headers, but should otherwise be OK I think.
>
> Some of these workarounds look like they would break if we add the missing
> PGDLLIMPORT in future releases.  That would be nasty :-(.  Am I misreading
> it?
>
>
I don't think they will, but without testing and more digging I can't be
sure. If marking the variable __declspec(dllexport) causes its import table
entry to be omitted then yes, that'd break things.

I'll try to dig out my Windows VM and prep a few tests once I've delivered
on the promised pglogical downstream.

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

Re: [HACKERS] problem with msvc linker - cannot build orafce

[Kisung Kim]

2015-11-24 8:12 GMT+09:00 Chapman Flack :

> On 11/23/15 15:14, Tom Lane wrote:
> > Lack of PGDLLIMPORT on the extern declaration, no doubt.
> >
> > The fact that we've not heard this before implies that either nobody has
> > ever tried to use orafce on Windows, or it only very recently grew a
> > dependency on session_timezone.
>
>
Actually, we encountered the situation before couple of months.
A client wanted to use orafce on Windows and the same build problem
occurred.
We performed a workaround to edit the PG source to export unresolved
symbols,
which I think of not a good solution.

2015-11-24 8:12 GMT+09:00 Chapman Flack :

> Has anyone got the stomach to try such a thing and see what happens?
> I don't have MSVC here.
>
> -Chap


We have the environment to test your ideas.
Can you explain your ideas with more detail?





(C)Bitnine, Kisung Kim, Ph.D
https://sites.google.com/site/kisungresearch/
E-mail : ks...@bitnine.co.kr
Office phone : 070-4800-3321
Mobile phone : 010-7136-0834
Fax : 02-568-1332

Re: [HACKERS] problem with msvc linker - cannot build orafce

[Chapman Flack]

On 11/24/2015 05:33 AM, Kisung Kim wrote:
> 2015-11-24 8:12 GMT+09:00 Chapman Flack :
>> On 11/23/15 15:14, Tom Lane wrote:
>>> Lack of PGDLLIMPORT on the extern declaration, no doubt.
>>
> Actually, we encountered the situation before couple of months.
> A client wanted to use orafce on Windows and the same build problem
> occurred.
> We performed a workaround to edit the PG source to export unresolved
> symbols,
> which I think of not a good solution.

>> Has anyone got the stomach to try such a thing and see what happens?
>> I don't have MSVC here.
> 
> We have the environment to test your ideas.
> Can you explain your ideas with more detail?

Well, the main idea is just this:  *IF* it is sufficient to declare
a variable PGDLLIMPORT only in the code that is importing it (the
big IF because I don't know whether that is true, but something I
saw in that long earlier thread seemed to suggest it) ...

Then ... the chief problem that needs to be solved is only that
MSVC won't allow you to redeclare something with PGDLLIMPORT if
it is also declared without PGDLLIMPORT in a .h file that you
include. In other words, you can't simply:

#include 
extern PGDLLIMPORT pg_tz session_timezone; /* the right way now */

because it was already declared the wrong way in pgtime.h.

So one idea is just this:

#define session_timezone decoy_session_timezone;
#include 
#undef decoy_session_timezone;

extern PGDLLIMPORT pg_tz session_timezone; /* the right way now */

which is not a multiple declaration of the same thing, because
what got declared the wrong way in pgtime.h is now some other thing
named decoy_session_timezone.  You might need to supply a thing by
that name, to avoid a linker complaint:

pg_tz decoy_session_timezone; /* never used */

IF the original premise is true, then this technique ought to be
usable in most cases. It would, however, break in cases where the
.h file declares macros or inline functions that refer to the
symbol, because they would all end up referring to the decoy.

My other idea, especially if there were several symbols needing
to be treated this way, would be to do it all in one dedicated
.c file, so any of the possible problems with #defining away parts
of an .h file would be contained in one place, and that file could
have a simple getter function:

pg_tz getSessionTimezone() { return session_timezone; }

which would be used in the rest of the code instead of referring
to the global directly. (In that case, of course, the same getter
function would have to be provided in the non-MSVC case too.)
A setter function could also be made, if the code needs it.



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

[HACKERS] problem with msvc linker - cannot build orafce

[Pavel Stehule]

Hi

I am trying to build Orafce and I have problem due access to exported
variable session_timezone.

The build fails with message:

1> Creating library
C:\Users\Pavel\orafce-VERSION_3_1_2\orafce-VERSION_3_1_2\msvc\/bin/x64/9.4/lib/orafce.lib
and object
C:\Users\Pavel\orafce-VERSION_3_1_2\orafce-VERSION_3_1_2\msvc\/bin/x64/9.4/lib/orafce.exp
1>datefce.obj : error LNK2001: unresolved external symbol session_timezone
1>C:\Users\Pavel\orafce-VERSION_3_1_2\orafce-VERSION_3_1_2\msvc\/bin/x64/9.4/lib//orafce.dll
: fatal error LNK1120: 1 unresolved externals

I am using msvc 2015 and PostgreSQL 9.4.5 from EnterpriseDB.

Any idea what can be broken?

Regards

Pavel

Re: [HACKERS] problem with msvc linker - cannot build orafce

[Tom Lane]

Pavel Stehule  writes:
> I am trying to build Orafce and I have problem due access to exported
> variable session_timezone.
> Any idea what can be broken?

Lack of PGDLLIMPORT on the extern declaration, no doubt.

The fact that we've not heard this before implies that either nobody has
ever tried to use orafce on Windows, or it only very recently grew a
dependency on session_timezone.

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] problem with msvc linker - cannot build orafce

[Chapman Flack]

On 11/23/15 15:14, Tom Lane wrote:
> Pavel Stehule  writes:
>> I am trying to build Orafce and I have problem due access to exported
>> variable session_timezone.
>> Any idea what can be broken?
> 
> Lack of PGDLLIMPORT on the extern declaration, no doubt.
> 
> The fact that we've not heard this before implies that either nobody has
> ever tried to use orafce on Windows, or it only very recently grew a
> dependency on session_timezone.

Or something changed in the build process. I've had Ken Olson report the
same symbol inaccessible when he builds PL/Java with MSVC, and he says
it has happened since PG 9.4.

I read myself to sleep about a week ago catching up on this old thread:

http://www.postgresql.org/message-id/24290.1391303...@sss.pgh.pa.us

What I (think I) took away from it was:

1.  Un-PGDLLIMPORTed references to global *functions* work ok.
Maybe they are thunked and a little less efficient, but they work.

2.  Un-PGDLLIMPORTed references to global *variables*, not so much.
They used to silently link (at least on some buildfarm critters)
but hold bogus data (maybe a thunk, taken as data?).

3.  The one concrete *action* taken in the course of that thread was to
tweak the build process to make sure such cases at least *fail*
because that's better than silent bogosity.

So it's possible that (3) is what makes both Orafce and PL/Java seem to
have started failing "recently" even though the code in question may be
years old (and for most of that time, while linking without complaint,
may have been bogus without someone noticing).

The question that interests me most right now: how, if at all, can the
extension author/maintainer work around this issue when it crops up?
Obviously, the Right Thing To Do is report it and get the PGDLLIMPORT
added here, but still for years to come the extension will have to cope
with being built against PG distributions that lack it. (Never mind the
reason, when the extension doesn't build, it's the extension that looks
bad.)

Now, I thought I spotted, somewhere in that long thread, the hint of an
idea that the magic works as long as the *extension* has the variable
declared PGDLLIMPORT, even if it wasn't declared that way when the PG
executable itself was built. Anybody else remember that, or did I
imagine it?

The snag seemed to be, MSVC won't tolerate two extern declarations, one
PGDLLIMPORT and one without, so you can't get away with including the .h
file (which might not have the PGDLLIMPORT) and then declaring it yourself.

You *might* get away with creating a separate C file (how about
chamberofhorrors.c?) that, rather revoltingly, *doesn't* include the
proper PostgreSQL .h files, only duplicates the necessary declarations
with PGDLLIMPORT added, and exports some getter/setter methods
to the rest of the extension code.  (I like the idea of one chamberofhorrors
better than scattering such rubbish all over the project.)

That might work ok for log_min_messages and client_min_messages, which are
just ints. Trickier maybe for session_timezone, which is a pg_tz, so you
can't really avoid pgtime.h. That leaves even more revolting options, like

#define session_timezone decoy_session_timezone
#include 
#undef session_timezone
extern PGDLLIMPORT pg_tz session_timezone

pg_tz decoy_session_timezone; /* unused */


Has anyone got the stomach to try such a thing and see what happens?
I don't have MSVC here.

-Chap


-- 
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] Problem with CREATE TABLE ... (LIKE ... INCLUDING INDEXES)

[Thom Brown]

On 14 June 2015 at 04:25, Michael Paquier michael.paqu...@gmail.com wrote:
 On Sun, Jun 14, 2015 at 11:38 AM, Thom Brown t...@linux.com wrote:
 As you can see, 3 indexes are missing, which happen to be ones that
 would duplicate the column definition of another index.  Is this
 intentional?  If so, shouldn't it be documented behaviour?

 Looking at the code (transformIndexConstraints in parse_utilcmd.c),
 this is intentional behavior:
 /*
  * Scan the index list and remove any redundant index
 specifications. This
  * can happen if, for instance, the user writes UNIQUE PRIMARY KEY. A
  * strict reading of SQL would suggest raising an error
 instead, but that
  * strikes me as too anal-retentive. - tgl 2001-02-14
  *
  * XXX in ALTER TABLE case, it'd be nice to look for duplicate
  * pre-existing indexes, too.
  */
 Per this commit:
 commit: c7d2ce7bc6eb02eac0c10fae9caf2936a71ad25c
 author: Tom Lane t...@sss.pgh.pa.us
 date: Wed, 14 Feb 2001 23:32:38 +
 Repair problems with duplicate index names generated when CREATE TABLE
 specifies redundant UNIQUE conditions.

 Perhaps a mention in the docs in the page of CREATE TABLE would be
 welcome. Something like Redundant index definitions are ignored with
 INCLUDING INDEXES.

 Thoughts?

The commit refers to duplicate index names, and only for UNIQUE
indexes.  This behaviour is beyond that.  And how does it determine
which index to copy?  In my example, I placed an index in a different
tablespace.  That could be on a drive with very different read/write
characteristics than the default tablespace (seek latency/sequential
read rate/write speed etc.) and possibly with different GUC
parameters, but there's no way for us to determine if this is the
case, so Postgres can easily remove the more performant one.

-- 
Thom


-- 
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] Problem with CREATE TABLE ... (LIKE ... INCLUDING INDEXES)

[Tom Lane]

Thom Brown t...@linux.com writes:
 The commit refers to duplicate index names, and only for UNIQUE
 indexes.  This behaviour is beyond that.  And how does it determine
 which index to copy?  In my example, I placed an index in a different
 tablespace.  That could be on a drive with very different read/write
 characteristics than the default tablespace (seek latency/sequential
 read rate/write speed etc.) and possibly with different GUC
 parameters, but there's no way for us to determine if this is the
 case, so Postgres can easily remove the more performant one.

TBH, I have no particular concern for this argument.  If you created
duplicate indexes you did a dumb thing anyway; you should not be expecting
that the system's response to that situation will be remarkably
intelligent.  As the comment indicates, the code in question is really
only meant to deal with a specific kind of redundancy we'd observed in
real-world CREATE TABLE commands.  It's probably accidental that it gets
applied in CREATE TABLE LIKE cases, but it doesn't bother me that it is.

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] Problem with CREATE TABLE ... (LIKE ... INCLUDING INDEXES)

[Thom Brown]

Hi all,

I've noticed that LIKE tablename INCLUDING INDEXES skips any indexes
that were duplicated.

e.g.

CREATE TABLE people (id int, alias text);
CREATE INDEX idx_people_id_1 ON people (id);
CREATE INDEX idx_people_id_2 ON people (id) WHERE id % 2 = 0;
CREATE INDEX idx_people_alias_1 ON people (alias);
CREATE INDEX idx_people_alias_2 ON people (alias);
CREATE INDEX idx_people_alias_3_tblspc ON people (alias) TABLESPACE ts;
CREATE INDEX idx_people_alias_4 ON people (alias) WITH (FILLFACTOR = 24);

\d+ people

Table public.people
 Column |  Type   | Modifiers | Storage  | Stats target | Description
+-+---+--+--+-
 id | integer |   | plain|  |
 alias  | text|   | extended |  |
Indexes:
idx_people_alias_1 btree (alias)
idx_people_alias_2 btree (alias)
idx_people_alias_3_tblspc btree (alias), tablespace ts
idx_people_alias_4 btree (alias) WITH (fillfactor=24)
idx_people_id_1 btree (id)
idx_people_id_2 btree (id) WHERE (id % 2) = 0




CREATE SCHEMA test;
CREATE TABLE test.people (LIKE people INCLUDING INDEXES);

\d+ test.people

 Table test.people
 Column |  Type   | Modifiers | Storage  | Stats target | Description
+-+---+--+--+-
 id | integer |   | plain|  |
 alias  | text|   | extended |  |
Indexes:
people_alias_idx btree (alias)
people_id_idx btree (id)
people_id_idx1 btree (id) WHERE (id % 2) = 0


As you can see, 3 indexes are missing, which happen to be ones that
would duplicate the column definition of another index.  Is this
intentional?  If so, shouldn't it be documented behaviour?

-- 
Thom


-- 
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] Problem with CREATE TABLE ... (LIKE ... INCLUDING INDEXES)

[Michael Paquier]

On Sun, Jun 14, 2015 at 11:38 AM, Thom Brown t...@linux.com wrote:
 As you can see, 3 indexes are missing, which happen to be ones that
 would duplicate the column definition of another index.  Is this
 intentional?  If so, shouldn't it be documented behaviour?

Looking at the code (transformIndexConstraints in parse_utilcmd.c),
this is intentional behavior:
/*
 * Scan the index list and remove any redundant index
specifications. This
 * can happen if, for instance, the user writes UNIQUE PRIMARY KEY. A
 * strict reading of SQL would suggest raising an error
instead, but that
 * strikes me as too anal-retentive. - tgl 2001-02-14
 *
 * XXX in ALTER TABLE case, it'd be nice to look for duplicate
 * pre-existing indexes, too.
 */
Per this commit:
commit: c7d2ce7bc6eb02eac0c10fae9caf2936a71ad25c
author: Tom Lane t...@sss.pgh.pa.us
date: Wed, 14 Feb 2001 23:32:38 +
Repair problems with duplicate index names generated when CREATE TABLE
specifies redundant UNIQUE conditions.

Perhaps a mention in the docs in the page of CREATE TABLE would be
welcome. Something like Redundant index definitions are ignored with
INCLUDING INDEXES.

Thoughts?
-- 
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: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Heikki Linnakangas]

On 05/06/2014 02:44 PM, Andres Freund wrote:

On 2014-05-05 13:41:00 +0300, Heikki Linnakangas wrote:

+/*
+ * Exit hook to unlock the global transaction entry we're working on.
+ */
+static void
+AtProcExit_Twophase(int code, Datum arg)
+{
+   /* same logic as abort */
+   AtAbort_Twophase();
+}
+
+/*
+ * Abort hook to unlock the global transaction entry we're working on.
+ */
+void
+AtAbort_Twophase(void)
+{
+   if (MyLockedGxact == NULL)
+   return;
+
+   /*
+* If we were in process of preparing the transaction, but haven't
+* written the WAL record yet, remove the global transaction entry.
+* Same if we are in the process of finishing an already-prepared
+* transaction, and fail after having already written the WAL 2nd
+* phase commit or rollback record.
+*
+* After that it's too late to abort, so just unlock the 
GlobalTransaction
+* entry.  We might not have transfered all locks and other state to the
+* prepared transaction yet, so this is a bit bogus, but it's the best 
we
+* can do.
+*/
+   if (!MyLockedGxact-valid)
+   {
+   RemoveGXact(MyLockedGxact);
+   }
+   else
+   {
+   LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
+
+   MyLockedGxact-locking_backend = InvalidBackendId;
+
+   LWLockRelease(TwoPhaseStateLock);
+   }
+   MyLockedGxact = NULL;
+}


Is it guaranteed that all paths have called LWLockReleaseAll()
before calling the proc exit hooks? Otherwise we might end up waiting
for ourselves...


Hmm. AbortTransaction() will release locks before we get here, but the 
before_shmem_exit() callpath will not. So an elog(FATAL), while holding 
TwoPhaseStateLock would cause us to deadlock with ourself. But there are 
no such elogs.


I copied this design from async.c, which is quite similar, so if there's 
a problem that ought to be fixed too. And there are other more 
complicated before_shmem callbacks that worry me more, like 
createdb_failure_callback(). But I think they're all all right.



  /*
   * MarkAsPreparing
@@ -261,29 +329,15 @@ MarkAsPreparing(TransactionId xid, const char *gid,
 errmsg(prepared transactions are disabled),
  errhint(Set max_prepared_transactions to a nonzero 
value.)));

-   LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
-
-   /*
-* First, find and recycle any gxacts that failed during prepare. We do
-* this partly to ensure we don't mistakenly say their GIDs are still
-* reserved, and partly so we don't fail on out-of-slots unnecessarily.
-*/
-   for (i = 0; i  TwoPhaseState-numPrepXacts; i++)
+   /* on first call, register the exit hook */
+   if (!twophaseExitRegistered)
{
-   gxact = TwoPhaseState-prepXacts[i];
-   if (!gxact-valid  !TransactionIdIsActive(gxact-locking_xid))
-   {
-   /* It's dead Jim ... remove from the active array */
-   TwoPhaseState-numPrepXacts--;
-   TwoPhaseState-prepXacts[i] = 
TwoPhaseState-prepXacts[TwoPhaseState-numPrepXacts];
-   /* and put it back in the freelist */
-   gxact-next = TwoPhaseState-freeGXacts;
-   TwoPhaseState-freeGXacts = gxact;
-   /* Back up index count too, so we don't miss scanning 
one */
-   i--;
-   }
+   before_shmem_exit(AtProcExit_Twophase, 0);
+   twophaseExitRegistered = true;
}


It's not particularly nice to register shmem exit hooks in the middle of
normal processing because it makes it impossible to use
cancel_before_shmem_exit() previously registered hooks. I think this
should be registered at startup, if max_prepared_xacts  0.


shrug. async.c and namespace.c does the same, and it hasn't been a 
problem.


I committed this now, but please let me know if you see a concrete 
problem with the locks.


- 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: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Andres Freund]

On 2014-05-15 17:21:28 +0300, Heikki Linnakangas wrote:
 Is it guaranteed that all paths have called LWLockReleaseAll()
 before calling the proc exit hooks? Otherwise we might end up waiting
 for ourselves...
 
 Hmm. AbortTransaction() will release locks before we get here, but the
 before_shmem_exit() callpath will not. So an elog(FATAL), while holding
 TwoPhaseStateLock would cause us to deadlock with ourself. But there are no
 such elogs.

 I copied this design from async.c, which is quite similar, so if there's a
 problem that ought to be fixed too. And there are other more complicated
 before_shmem callbacks that worry me more, like createdb_failure_callback().
 But I think they're all all right.

Perhaps we should enforce that LWLockReleaseAll() is called first?
E.g. in shmem_exit()? It'll happen in ProcKill() atm, but that's
normally pretty much at the bottom of the stack.

 It's not particularly nice to register shmem exit hooks in the middle of
 normal processing because it makes it impossible to use
 cancel_before_shmem_exit() previously registered hooks. I think this
 should be registered at startup, if max_prepared_xacts  0.
 
 shrug. async.c and namespace.c does the same, and it hasn't been a
 problem.

Well, it doesn't seem unreasonable to have C code using
PG_ENSURE_ERROR_CLEANUP/PG_END_ENSURE_ERROR_CLEANUP around a 2pc commit
to me. That'll break with this.
Perhaps we should just finally make cancel_before_shmem_exit search the
stack of callbacks.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

[Heikki Linnakangas]

On 04/14/2014 11:55 AM, Marko Kreen wrote:

On Sun, Apr 13, 2014 at 05:46:20PM -0400, Jan Wieck wrote:

On 04/13/14 14:22, Jan Wieck wrote:

On 04/13/14 08:27, Marko Kreen wrote:

I think you need to do SET_VARSIZE also here.  Alternative is to
move SET_VARSIZE after sort_snapshot().

And it seems the drop-double-txid logic should be added also to
txid_snapshot_recv().  It seems weird to have it behave differently

from txid_snapshot_in().




Thanks,

yes on both issues. Will create another patch.


New patch attached.

New github commit is 
https://github.com/wieck/postgres/commit/b8fd0d2eb78791e5171e34aecd233fd06218890d


Looks OK to me.


Ok, committed.

- 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: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Robert Haas]

On Thu, May 15, 2014 at 10:38 AM, Andres Freund and...@2ndquadrant.com wrote:
 shrug. async.c and namespace.c does the same, and it hasn't been a
 problem.

 Well, it doesn't seem unreasonable to have C code using
 PG_ENSURE_ERROR_CLEANUP/PG_END_ENSURE_ERROR_CLEANUP around a 2pc commit
 to me. That'll break with this.
 Perhaps we should just finally make cancel_before_shmem_exit search the
 stack of callbacks.

Yes, please.  And while we're at it, perhaps we should make it Trap()
or fail an Assert() if it doesn't find the callback it was told to
remove.

-- 
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: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Andres Freund]

Hi,

On 2014-05-05 13:41:00 +0300, Heikki Linnakangas wrote:
 I came up with the attached fix for this. Currently, the entry is implicitly
 considered dead or unlocked if the locking_xid transaction is no longer
 active, but this patch essentially turns locking_xid into a simple boolean,
 and makes it the backend's responsibility to clear it on abort. (it's not
 actually a boolean, it's a BackendId, but that's just for debugging purposes
 to track who's keeping the entry locked). This requires a process exit hook,
 and an abort hook, to make sure the entry is always released, but that's not
 too difficult. It allows the backend to release the entry at exactly the
 right time, instead of having it implicitly released by

 I considered Andres' idea of using a new heavy-weight lock, but didn't like
 it much. It would be a larger patch, which is not nice for back-patching.
 One issue would be that if you run out of lock memory, you could not roll
 back any prepared transactions, which is not nice because it could be a
 prepared transaction that's hoarding the lock memory.

I am not convinced by the latter reasoning but you're right that any
such change would hardly be backpatchable.

 +/*
 + * Exit hook to unlock the global transaction entry we're working on.
 + */
 +static void
 +AtProcExit_Twophase(int code, Datum arg)
 +{
 + /* same logic as abort */
 + AtAbort_Twophase();
 +}
 +
 +/*
 + * Abort hook to unlock the global transaction entry we're working on.
 + */
 +void
 +AtAbort_Twophase(void)
 +{
 + if (MyLockedGxact == NULL)
 + return;
 +
 + /*
 +  * If we were in process of preparing the transaction, but haven't
 +  * written the WAL record yet, remove the global transaction entry.
 +  * Same if we are in the process of finishing an already-prepared
 +  * transaction, and fail after having already written the WAL 2nd
 +  * phase commit or rollback record.
 +  *
 +  * After that it's too late to abort, so just unlock the 
 GlobalTransaction
 +  * entry.  We might not have transfered all locks and other state to the
 +  * prepared transaction yet, so this is a bit bogus, but it's the best 
 we
 +  * can do.
 +  */
 + if (!MyLockedGxact-valid)
 + {
 + RemoveGXact(MyLockedGxact);
 + }
 + else
 + {
 + LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
 +
 + MyLockedGxact-locking_backend = InvalidBackendId;
 +
 + LWLockRelease(TwoPhaseStateLock);
 + }
 + MyLockedGxact = NULL;
 +}

Is it guaranteed that all paths have called LWLockReleaseAll()
before calling the proc exit hooks? Otherwise we might end up waiting
for ourselves...

  /*
   * MarkAsPreparing
 @@ -261,29 +329,15 @@ MarkAsPreparing(TransactionId xid, const char *gid,
errmsg(prepared transactions are disabled),
 errhint(Set max_prepared_transactions to a nonzero 
 value.)));
  
 - LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
 -
 - /*
 -  * First, find and recycle any gxacts that failed during prepare. We do
 -  * this partly to ensure we don't mistakenly say their GIDs are still
 -  * reserved, and partly so we don't fail on out-of-slots unnecessarily.
 -  */
 - for (i = 0; i  TwoPhaseState-numPrepXacts; i++)
 + /* on first call, register the exit hook */
 + if (!twophaseExitRegistered)
   {
 - gxact = TwoPhaseState-prepXacts[i];
 - if (!gxact-valid  !TransactionIdIsActive(gxact-locking_xid))
 - {
 - /* It's dead Jim ... remove from the active array */
 - TwoPhaseState-numPrepXacts--;
 - TwoPhaseState-prepXacts[i] = 
 TwoPhaseState-prepXacts[TwoPhaseState-numPrepXacts];
 - /* and put it back in the freelist */
 - gxact-next = TwoPhaseState-freeGXacts;
 - TwoPhaseState-freeGXacts = gxact;
 - /* Back up index count too, so we don't miss scanning 
 one */
 - i--;
 - }
 + before_shmem_exit(AtProcExit_Twophase, 0);
 + twophaseExitRegistered = true;
   }

It's not particularly nice to register shmem exit hooks in the middle of
normal processing because it makes it impossible to use
cancel_before_shmem_exit() previously registered hooks. I think this
should be registered at startup, if max_prepared_xacts  0.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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

Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Heikki Linnakangas]

On 04/14/2014 09:48 PM, Heikki Linnakangas wrote:

On 04/14/2014 07:51 PM, Tom Lane wrote:

I'd prefer to leave the prepare sequence alone and instead find a way
to reject COMMIT PREPARED until after the source transaction is safely
clear of the race conditions.  The upthread idea of looking at vxid
instead of xid might help, except that I see we clear both of them
in ProcArrayClearTransaction.  We'd need some state in PGPROC that
isn't cleared till later than that.


Hmm. What if one of the post-cleanup action fails? We can't bail out of
the prepare sequence until we have transfered the locks to the new
PGPROC. Otherwise the locks are lost. In essence, there should be a
critical section from the EndPrepare call until all the critical cleanup
actions like PostPrepare_Locks have been done, and I don't think we want
that. We might be able to guarantee that the built-in post-cleanup
operations are safe enough for that, but there's also CallXactCallbacks
in there.

Given the lack of reports of that happening, though, perhaps that's not
an issue.


I came up with the attached fix for this. Currently, the entry is 
implicitly considered dead or unlocked if the locking_xid transaction is 
no longer active, but this patch essentially turns locking_xid into a 
simple boolean, and makes it the backend's responsibility to clear it on 
abort. (it's not actually a boolean, it's a BackendId, but that's just 
for debugging purposes to track who's keeping the entry locked). This 
requires a process exit hook, and an abort hook, to make sure the entry 
is always released, but that's not too difficult. It allows the backend 
to release the entry at exactly the right time, instead of having it 
implicitly released by ProcArrayClearTransaction.


If we error during prepare, after having written the prepare WAL record 
but before the locks have been transfered to the dummy PGPROC, the locks 
are simply released. This is wrong, but it's always been like that and 
we haven't heard any complaints of that from the field, so I'm inclined 
to leave it as it is. We could use a critical section to force a panic, 
but that cure could be a worse than the disease.


I considered Andres' idea of using a new heavy-weight lock, but didn't 
like it much. It would be a larger patch, which is not nice for 
back-patching. One issue would be that if you run out of lock memory, 
you could not roll back any prepared transactions, which is not nice 
because it could be a prepared transaction that's hoarding the lock memory.


- Heikki

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 66dbf58..995f51f 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -58,6 +58,7 @@
 #include replication/walsender.h
 #include replication/syncrep.h
 #include storage/fd.h
+#include storage/ipc.h
 #include storage/predicate.h
 #include storage/proc.h
 #include storage/procarray.h
@@ -82,25 +83,25 @@ int			max_prepared_xacts = 0;
  *
  * The lifecycle of a global transaction is:
  *
- * 1. After checking that the requested GID is not in use, set up an
- * entry in the TwoPhaseState-prepXacts array with the correct XID and GID,
- * with locking_xid = my own XID and valid = false.
+ * 1. After checking that the requested GID is not in use, set up an entry in
+ * the TwoPhaseState-prepXacts array with the correct GID and valid = false,
+ * and mark it as locked by my backend.
  *
  * 2. After successfully completing prepare, set valid = true and enter the
  * referenced PGPROC into the global ProcArray.
  *
- * 3. To begin COMMIT PREPARED or ROLLBACK PREPARED, check that the entry
- * is valid and its locking_xid is no longer active, then store my current
- * XID into locking_xid.  This prevents concurrent attempts to commit or
- * rollback the same prepared xact.
+ * 3. To begin COMMIT PREPARED or ROLLBACK PREPARED, check that the entry is
+ * valid and not locked, then mark the entry as locked by storing my current
+ * backend ID into locking_backend.  This prevents concurrent attempts to
+ * commit or rollback the same prepared xact.
  *
  * 4. On completion of COMMIT PREPARED or ROLLBACK PREPARED, remove the entry
  * from the ProcArray and the TwoPhaseState-prepXacts array and return it to
  * the freelist.
  *
  * Note that if the preparing transaction fails between steps 1 and 2, the
- * entry will remain in prepXacts until recycled.  We can detect recyclable
- * entries by checking for valid = false and locking_xid no longer active.
+ * entry must be removed so that the GID and the GlobalTransaction struct
+ * can be reused.  See AtAbort_Twophase().
  *
  * typedef struct GlobalTransactionData *GlobalTransaction appears in
  * twophase.h
@@ -115,8 +116,8 @@ typedef struct GlobalTransactionData
 	TimestampTz prepared_at;	/* time of preparation */
 	XLogRecPtr	prepare_lsn;	/* XLOG offset of prepare record */
 	Oid			owner;			/* ID of user that executed the xact */
-	

Re: [HACKERS] Problem with displaying wide tables in psql

[Greg Stark]

On Tue, Apr 29, 2014 at 2:52 AM, Peter Eisentraut pete...@gmx.net wrote:
 Please fix this compiler warning.  I think it came from this patch.


Oops, I fixed it in a previous version but didn't notice it had crept
back in in the back-and-forth. Will do.

-- 
greg


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

Re: [HACKERS] Problem with displaying wide tables in psql

[Peter Eisentraut]

Please fix this compiler warning.  I think it came from this patch.


print.c: In function ‘print_aligned_vertical’:
print.c:1354:10: error: pointer targets in passing argument 1 of 
‘strlen_max_width’ differ in signedness [-Werror=pointer-sign]
  encoding);
  ^
print.c:126:12: note: expected ‘unsigned char *’ but argument is of type ‘char 
*’
 static int strlen_max_width(unsigned char *str, int *target_width, int 
encoding);
^




-- 
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] Problem with displaying wide tables in psql

[Michael Paquier]

On Tue, Apr 29, 2014 at 12:37 PM, Sergey Muraviov
sergey.k.murav...@gmail.com wrote:
 2014-04-29 5:52 GMT+04:00 Peter Eisentraut pete...@gmx.net:

 Please fix this compiler warning.  I think it came from this patch.
 print.c: In function 'print_aligned_vertical':
 print.c:1354:10: error: pointer targets in passing argument 1 of
 'strlen_max_width' differ in signedness [-Werror=pointer-sign]
   encoding);
   ^
 print.c:126:12: note: expected 'unsigned char *' but argument is of type
 'char *'
  static int strlen_max_width(unsigned char *str, int *target_width, int
 encoding);
 ^
 fixed
Your patch has been committed by Greg not so long ago, you should send
for this warning a patch rebased on the latest master branch commit :)
-- 
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] Problem with displaying wide tables in psql

[Sergey Muraviov]

rebased


2014-04-29 7:43 GMT+04:00 Michael Paquier michael.paqu...@gmail.com:

 On Tue, Apr 29, 2014 at 12:37 PM, Sergey Muraviov
 sergey.k.murav...@gmail.com wrote:
  2014-04-29 5:52 GMT+04:00 Peter Eisentraut pete...@gmx.net:
 
  Please fix this compiler warning.  I think it came from this patch.
  print.c: In function 'print_aligned_vertical':
  print.c:1354:10: error: pointer targets in passing argument 1 of
  'strlen_max_width' differ in signedness [-Werror=pointer-sign]
encoding);
^
  print.c:126:12: note: expected 'unsigned char *' but argument is of type
  'char *'
   static int strlen_max_width(unsigned char *str, int *target_width, int
  encoding);
  ^
  fixed
 Your patch has been committed by Greg not so long ago, you should send
 for this warning a patch rebased on the latest master branch commit :)
 --
 Michael




-- 
Best regards,
Sergey Muraviov
diff --git a/src/bin/psql/print.c b/src/bin/psql/print.c
index 0eb..b2f56a3 100644
--- a/src/bin/psql/print.c
+++ b/src/bin/psql/print.c
@@ -1350,8 +1350,7 @@ print_aligned_vertical(const printTableContent *cont, FILE *fout)
 			{
 int swidth, twidth = hwidth + 1;
 fputs(hline? format-header_nl_left:  , fout);
-strlen_max_width((char *) hlineptr[hline].ptr, twidth,
- encoding);
+strlen_max_width(hlineptr[hline].ptr, twidth, encoding);
 fprintf(fout, %-s, hlineptr[hline].ptr);
 
 swidth = hwidth - twidth;

-- 
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] Problem with displaying wide tables in psql

[Tom Lane]

Greg Stark st...@mit.edu writes:
 I expect this regression test to fail on platforms that don't support
 utf-8 client-side (I'm assuming we such things?). I don't have such a
 platform here and I'm not sure how it would fail so I want to go ahead
 and apply it and grab the output to add the alternate output when it
 fails on the build-farm. Would that be ok?

Are you expecting to carry an alternate expected file for every possible
encoding choice?  That does not seem workable to me, and even if we could
do it the cost/benefit ratio would be pretty grim.  I think you should
drop the UTF8-dependent tests.

In other words: there are no encoding dependencies in the existing
standard regression tests.  This feature is not the place to start adding
them, and two weeks past feature freeze is not the time to start adding
them either.  We don't have time right now to shake out a whole new
set of platform dependencies in the regression tests.

If you feel these tests must be preserved someplace, you could add a
new regression test that isn't run by default, following in the
footsteps of collate.linux.utf8.

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] Problem with displaying wide tables in psql

[Greg Stark]

Not sure what other encodings you mean. Psql uses utf8 for the border and
the test uses utf8 to test the formatting. I was only anticipating an error
on platforms where that didn't work.

I would lean towards having it but I'm fine following your judgement,
especially given the timing.

-- 
greg

Re: [HACKERS] Problem with displaying wide tables in psql

[Tom Lane]

Greg Stark st...@mit.edu writes:
 Not sure what other encodings you mean. Psql uses utf8 for the border and
 the test uses utf8 to test the formatting. I was only anticipating an error
 on platforms where that didn't work.

Well, there are two likely misbehaviors if the regression test is being
run in some other encoding:

1. If it's a single-byte encoding, you probably won't get any bad-encoding
complaints, but the code will think the utf8 characters represent multiple
logical characters, resulting in (at least) spacing differences.  It's
possible that all single-byte encodings would act the same, but I'm not
sure.

2. If it's a multi-byte encoding different from utf8, you're almost
certainly going to get badly-encoded-data complaints, at different places
depending on the particular encoding.

I don't remember how many different multibyte encodings we support,
but I'm pretty sure we'd need a separate expected file for each one.
Plus at least one for the single-byters.

The real problem is that I don't have a lot of confidence that the
buildfarm would provide us with full coverage of all the encodings
that somebody might use in the field.  So we might not find out about
omissions or wrong expected-files until after we ship.

Anyway, the bottom line for me is that this test isn't worth that
much trouble.  I'm okay with putting it in as a separate test file
that we don't support running in non-utf8 encodings.

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: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Heikki Linnakangas]

On 04/13/2014 11:39 PM, Heikki Linnakangas wrote:

However, I just noticed that there's a race condition between PREPARE
TRANSACTION and COMMIT/ROLLBACK PREPARED. PostPrepare_Locks runs after
the prepared transaction is already marked as fully prepared. That means
that by the time we get to PostPrepare_Locks, another backend might
already have finished and removed the prepared transaction. That leads
to a PANIC (put a breakpoint just before PostPrepare_Locks):

postgres=# commit prepared 'foo';
PANIC:  failed to re-find shared proclock object
PANIC:  failed to re-find shared proclock object
The connection to the server was lost. Attempting reset: Failed.

FinishPrepareTransaction reads the list of locks from the two-phase
state file, but PANICs when it doesn't find the corresponding locks in
the lock manager (because PostPrepare_Locks hasn't transfered them to
the dummy PGPROC yet).

I think we'll need to transfer of the locks earlier, before the
transaction is marked as fully prepared. I'll take a closer look at this
tomorrow.


Here's a patch to do that. It's very straightforward, I just moved the 
calls to transfer locks earlier, before ProcArrayClearTransaction. 
PostPrepare_MultiXact had a similar race -  it also transfer state from 
the old PGPROC entry to the new, and needs to be done before allowing 
another backend to remove the new PGPROC entry. I changed the names of 
the functions to distinguish them from the other PostPrepare_* functions 
that now happen at a different time.


The patch is simple, but it's a bit scary to change the order of things 
like this. Looking at all the calls that now happen after transferring 
the locks, I believe this is OK. The change also applies to the 
callbacks called by the RegisterXactCallback mechanism, which means that 
in theory there might be a 3rd party extension out there that's 
affected. All the callbacks in contrib and plpgsql are OK, and it's 
questionable to do anything complicated that would depend on 
heavy-weight locks to be held in those callbacks, so I think this is OK. 
Warrants a note in the release notes, though.


- Heikki

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index d4ad678..b505c62 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1580,11 +1580,12 @@ AtPrepare_MultiXact(void)
 }
 
 /*
- * PostPrepare_MultiXact
- *		Clean up after successful PREPARE TRANSACTION
+ * TransferPrepare_MultiXact
+ *		Called after successful PREPARE TRANSACTION, before releasing our
+ *		PGPROC entry.
  */
 void
-PostPrepare_MultiXact(TransactionId xid)
+TransferPrepare_MultiXact(TransactionId xid)
 {
 	MultiXactId myOldestMember;
 
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index b20d973..c60edf1 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2224,6 +2224,16 @@ PrepareTransaction(void)
 	XactLastRecEnd = 0;
 
 	/*
+	 * Transfer any heavy-weight locks we're holding to the dummy ProcArray.
+	 *
+	 * NB: this has to be done before clearing our own ProcArray entry.
+	 * This is different from normal commit, where locks are released after
+	 * clearing the ProcArray entry!
+	 */
+	TransferPrepare_MultiXact(xid);	  /* also transfer our multixact state */
+	TransferPrepare_Locks(xid);
+
+	/*
 	 * Let others know about no transaction in progress by me.	This has to be
 	 * done *after* the prepared transaction has been marked valid, else
 	 * someone may think it is unlocked and recyclable.
@@ -2234,6 +2244,10 @@ PrepareTransaction(void)
 	 * This is all post-transaction cleanup.  Note that if an error is raised
 	 * here, it's too late to abort the transaction.  This should be just
 	 * noncritical resource releasing.	See notes in CommitTransaction.
+	 *
+	 * NB: we already transfered the locks to the prepared ProcArray entry,
+	 * so even the cleanup before ResourceOwnerRelease(RESOURCE_RELEASE_LOCKS)
+	 * cannot rely on any heavy-weight locks being held!
 	 */
 
 	CallXactCallbacks(XACT_EVENT_PREPARE);
@@ -2256,11 +2270,12 @@ PrepareTransaction(void)
 
 	PostPrepare_smgr();
 
-	PostPrepare_MultiXact(xid);
-
-	PostPrepare_Locks(xid);
 	PostPrepare_PredicateLocks(xid);
 
+	/*
+	 * we're not actually holding any locks anymore, but clean up any other
+	 * resources that might need to be cleaned up at this stage.
+	 */
 	ResourceOwnerRelease(TopTransactionResourceOwner,
 		 RESOURCE_RELEASE_LOCKS,
 		 true, true);
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 6825063..779f0cb 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -3069,8 +3069,8 @@ AtPrepare_Locks(void)
 }
 
 /*
- * PostPrepare_Locks
- *		Clean up after successful PREPARE
+ * TransferPrepare_Locks
+ *		Transfer locks to prepared transaction after successful PREPARE
  *
  * Here, we want to transfer ownership of our locks to a dummy PGPROC
  

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

[Marko Kreen]

On Sun, Apr 13, 2014 at 05:46:20PM -0400, Jan Wieck wrote:
 On 04/13/14 14:22, Jan Wieck wrote:
 On 04/13/14 08:27, Marko Kreen wrote:
 I think you need to do SET_VARSIZE also here.  Alternative is to
 move SET_VARSIZE after sort_snapshot().
 
 And it seems the drop-double-txid logic should be added also to
 txid_snapshot_recv().  It seems weird to have it behave differently
 from txid_snapshot_in().
 
 
 Thanks,
 
 yes on both issues. Will create another patch.
 
 New patch attached.
 
 New github commit is 
 https://github.com/wieck/postgres/commit/b8fd0d2eb78791e5171e34aecd233fd06218890d

Looks OK to me.

-- 
marko



-- 
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] Problem with txid_snapshot_in/out() functionality

[Heikki Linnakangas]

On 04/12/2014 05:03 PM, Andres Freund wrote:

On 2014-04-12 09:47:24 -0400, Tom Lane wrote:

Heikki Linnakangas hlinnakan...@vmware.com writes:

On 04/12/2014 12:07 AM, Jan Wieck wrote:

the Slony team has been getting seldom reports of a problem with the
txid_snapshot data type.
The symptom is that a txid_snapshot on output lists the same txid
multiple times in the xip list part of the external representation.



It's two-phase commit. When preparing a transaction, the state of the
transaction is first transfered to a dummy PGXACT entry, and then the
PGXACT entry of the backend is cleared. There is a transient state when
both PGXACT entries have the same xid.


Hm, yeah, but why is that intermediate state visible to anyone else?
Don't we have exclusive lock on the PGPROC array while we're doing this?


It's done outside the remit of ProcArray lock :(. And documented to lead
to duplicate xids in PGXACT.
EndPrepare():
/*
 * Mark the prepared transaction as valid.  As soon as xact.c marks
 * MyPgXact as not running our XID (which it will do immediately after
 * this function returns), others can commit/rollback the xact.
 *
 * NB: a side effect of this is to make a dummy ProcArray entry for the
 * prepared XID.  This must happen before we clear the XID from 
MyPgXact,
 * else there is a window where the XID is not running according to
 * TransactionIdIsInProgress, and onlookers would be entitled to assume
 * the xact crashed.  Instead we have a window where the same XID 
appears
 * twice in ProcArray, which is OK.
 */
MarkAsPrepared(gxact);

It doesn't sound too hard to essentially move PrepareTransaction()'s
ProcArrayClearTransaction() into MarkAsPrepared() and rejigger the
locking to remove the intermediate state. But I think it'll lead to
bigger changes than we'd be comfortable backpatching.


Hmm. There's a field in GlobalTransactionData called locking_xid, which 
is used to mark the XID of the transaction that's currently operating on 
the prepared transaction. At prepare, that ensures that the transaction 
cannot be committed or rolled back by another backend until the original 
backend has cleared its PGPROC entry. At COMMIT/ROLLBACK PREPARED, it 
ensures that only one backend can commit/rollback the transaction.


I wonder why we don't use a VirtualTransactionId there. AFAICS there is 
no reason for COMMIT/ROLLBACK PREPARED to be assigned an XID of its own. 
And if we used a VirtualTransactionId there, prepare could clear the xid 
field of the PGPROC entry earlier.


- 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] Problem with txid_snapshot_in/out() functionality

[Andres Freund]

On 2014-04-14 12:15:30 +0300, Heikki Linnakangas wrote:
 Hmm. There's a field in GlobalTransactionData called locking_xid, which is
 used to mark the XID of the transaction that's currently operating on the
 prepared transaction. At prepare, that ensures that the transaction cannot
 be committed or rolled back by another backend until the original backend
 has cleared its PGPROC entry. At COMMIT/ROLLBACK PREPARED, it ensures that
 only one backend can commit/rollback the transaction.
 
 I wonder why we don't use a VirtualTransactionId there.

I wondered about that previously as well. My bet it's because the 2pc
support arrived before the virtualxact stuff...

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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

Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Tom Lane]

Heikki Linnakangas hlinnakan...@vmware.com writes:
 I think we'll need to transfer of the locks earlier, before the
 transaction is marked as fully prepared. I'll take a closer look at this
 tomorrow.

 Here's a patch to do that. It's very straightforward, I just moved the 
 calls to transfer locks earlier, before ProcArrayClearTransaction. 
 PostPrepare_MultiXact had a similar race -  it also transfer state from 
 the old PGPROC entry to the new, and needs to be done before allowing 
 another backend to remove the new PGPROC entry. I changed the names of 
 the functions to distinguish them from the other PostPrepare_* functions 
 that now happen at a different time.

Why didn't you also move up PostPrepare_PredicateLocks?  Seems like its
access to MySerializableXact is also racy.

 The patch is simple, but it's a bit scary to change the order of things 
 like this.

Yeah.  There are a lot of assumptions in there about the order of resource
release, in particular that it is safe to do certain things because we're
still holding locks.

I poked around a bit and noticed one theoretical problem sequence: if the
prepared xact drops some relation that we're still holding buffer pins on.
This shouldn't really happen (why are we still pinning some rel we think
we dropped?) but if it did, the commit would do DropRelFileNodeBuffers
which would end up busy-looping until we drop our pins (see
InvalidateBuffer, which thinks this must be an I/O wait situation).
So it would work, more or less, but it seems pretty fragile.  I'm afraid
there are more assumptions like this one.

The whole thing feels like we are solving the wrong problem, anyway.
IIUC, the complaint arises because we are allowing COMMIT PREPARED
to occur before the source transaction has reported successful prepare
to its client.  Surely that does not need to be a legal case?  No
correctly-operating 2PC xact manager would do that.

I'd prefer to leave the prepare sequence alone and instead find a way
to reject COMMIT PREPARED until after the source transaction is safely
clear of the race conditions.  The upthread idea of looking at vxid
instead of xid might help, except that I see we clear both of them
in ProcArrayClearTransaction.  We'd need some state in PGPROC that
isn't cleared till later than that.

regards, tom lane


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

Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Andres Freund]

On 2014-04-14 12:51:02 -0400, Tom Lane wrote:
 The whole thing feels like we are solving the wrong problem, anyway.
 IIUC, the complaint arises because we are allowing COMMIT PREPARED
 to occur before the source transaction has reported successful prepare
 to its client.  Surely that does not need to be a legal case?  No
 correctly-operating 2PC xact manager would do that.

I agree here. This seems somewhat risky, just to support a case that
shouldn't happen in reality - as somewhat evidenced by the fact that
there don't seem to be field reports around this.

 The upthread idea of looking at vxid
 instead of xid might help, except that I see we clear both of them
 in ProcArrayClearTransaction.  We'd need some state in PGPROC that
 isn't cleared till later than that.

I wonder if the most natural way to express this wouldn't be to have a
heavyweight lock for every 2pc xact
'slot'. ResourceOwnerRelease(RESOURCE_RELEASE_LOCKS) should be scheduled
correctly to make error handling for this work.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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

Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Tom Lane]

Andres Freund and...@2ndquadrant.com writes:
 I wonder if the most natural way to express this wouldn't be to have a
 heavyweight lock for every 2pc xact
 'slot'. ResourceOwnerRelease(RESOURCE_RELEASE_LOCKS) should be scheduled
 correctly to make error handling for this work.

That seems like not a bad idea.  Could we also use the same lock to
prevent concurrent attempts to commit/rollback the same already-prepared
transaction?  I forget what we're doing to forestall such cases right now.

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: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Andres Freund]

On 2014-04-14 13:47:35 -0400, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  I wonder if the most natural way to express this wouldn't be to have a
  heavyweight lock for every 2pc xact
  'slot'. ResourceOwnerRelease(RESOURCE_RELEASE_LOCKS) should be scheduled
  correctly to make error handling for this work.
 
 That seems like not a bad idea.  Could we also use the same lock to
 prevent concurrent attempts to commit/rollback the same already-prepared
 transaction?  I forget what we're doing to forestall such cases right now.

GlobalTransaction-locking_xid is currently used. If it points to a live
transaction by another backned prepared transaction with identifier
\%s\ is busy will be thrown.
ISTM if there were using a lock for every slot, that logic couldbe
thrown away.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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

Re: Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Heikki Linnakangas]

On 04/14/2014 07:51 PM, Tom Lane wrote:

I'd prefer to leave the prepare sequence alone and instead find a way
to reject COMMIT PREPARED until after the source transaction is safely
clear of the race conditions.  The upthread idea of looking at vxid
instead of xid might help, except that I see we clear both of them
in ProcArrayClearTransaction.  We'd need some state in PGPROC that
isn't cleared till later than that.


Hmm. What if one of the post-cleanup action fails? We can't bail out of 
the prepare sequence until we have transfered the locks to the new 
PGPROC. Otherwise the locks are lost. In essence, there should be a 
critical section from the EndPrepare call until all the critical cleanup 
actions like PostPrepare_Locks have been done, and I don't think we want 
that. We might be able to guarantee that the built-in post-cleanup 
operations are safe enough for that, but there's also CallXactCallbacks 
in there.


Given the lack of reports of that happening, though, perhaps that's not 
an issue.


- 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] Problem with txid_snapshot_in/out() functionality

[Marko Kreen]

On Sat, Apr 12, 2014 at 02:10:13PM -0400, Jan Wieck wrote:
 Since it doesn't seem to produce any side effects, I'd think that
 making the snapshot unique within txid_current_snapshot() and
 filtering duplicates on input should be sufficient and eligible for
 backpatching.

Agreed.

 The attached patch adds a unique loop to the internal
 sort_snapshot() function and skips duplicates on input. The git
 commit is here:
 
 https://github.com/wieck/postgres/commit/a88a2b2c25b856478d7e2b012fc718106338fe00

   static void
   sort_snapshot(TxidSnapshot *snap)
   {
 + txidlast = 0;
 + int nxip, idx1, idx2;
 + 
   if (snap-nxip  1)
 + {
   qsort(snap-xip, snap-nxip, sizeof(txid), cmp_txid);
 + nxip = snap-nxip;
 + idx1 = idx2 = 0;
 + while (idx1  nxip)
 + {
 + if (snap-xip[idx1] != last)
 + last = snap-xip[idx2++] = snap-xip[idx1];
 + else
 + snap-nxip--;
 + idx1++;
 + }
 + }
   }

I think you need to do SET_VARSIZE also here.  Alternative is to
move SET_VARSIZE after sort_snapshot().

And it seems the drop-double-txid logic should be added also to
txid_snapshot_recv().  It seems weird to have it behave differently
from txid_snapshot_in().

-- 
marko



-- 
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] Problem with txid_snapshot_in/out() functionality

[Jan Wieck]

On 04/13/14 08:27, Marko Kreen wrote:

On Sat, Apr 12, 2014 at 02:10:13PM -0400, Jan Wieck wrote:

Since it doesn't seem to produce any side effects, I'd think that
making the snapshot unique within txid_current_snapshot() and
filtering duplicates on input should be sufficient and eligible for
backpatching.


Agreed.


The attached patch adds a unique loop to the internal
sort_snapshot() function and skips duplicates on input. The git
commit is here:

https://github.com/wieck/postgres/commit/a88a2b2c25b856478d7e2b012fc718106338fe00



  static void
  sort_snapshot(TxidSnapshot *snap)
  {
+   txidlast = 0;
+   int nxip, idx1, idx2;
+
if (snap-nxip  1)
+   {
qsort(snap-xip, snap-nxip, sizeof(txid), cmp_txid);
+   nxip = snap-nxip;
+   idx1 = idx2 = 0;
+   while (idx1  nxip)
+   {
+   if (snap-xip[idx1] != last)
+   last = snap-xip[idx2++] = snap-xip[idx1];
+   else
+   snap-nxip--;
+   idx1++;
+   }
+   }
  }


I think you need to do SET_VARSIZE also here.  Alternative is to
move SET_VARSIZE after sort_snapshot().

And it seems the drop-double-txid logic should be added also to
txid_snapshot_recv().  It seems weird to have it behave differently
from txid_snapshot_in().



Thanks,

yes on both issues. Will create another patch.


Jan

--
Jan Wieck
Senior Software Engineer
http://slony.info


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

Race condition between PREPARE TRANSACTION and COMMIT PREPARED (was Re: [HACKERS] Problem with txid_snapshot_in/out() functionality)

[Heikki Linnakangas]

On 04/12/2014 05:03 PM, Andres Freund wrote:

If we don't, aren't we letting other backends see non-self-consistent
state in regards to who holds which locks, for example?

I think that actually works out ok, because the locks aren't owned by
xids/xacts, but procs. Otherwise we'd be in deep trouble in
CommitTransaction() as well where ProcArrayEndTransaction() clearing
that state.
After the whole xid transfer, there's PostPrepare_Locks() transferring
the locks.


Right.

However, I just noticed that there's a race condition between PREPARE 
TRANSACTION and COMMIT/ROLLBACK PREPARED. PostPrepare_Locks runs after 
the prepared transaction is already marked as fully prepared. That means 
that by the time we get to PostPrepare_Locks, another backend might 
already have finished and removed the prepared transaction. That leads 
to a PANIC (put a breakpoint just before PostPrepare_Locks):


postgres=# commit prepared 'foo';
PANIC:  failed to re-find shared proclock object
PANIC:  failed to re-find shared proclock object
The connection to the server was lost. Attempting reset: Failed.

FinishPrepareTransaction reads the list of locks from the two-phase 
state file, but PANICs when it doesn't find the corresponding locks in 
the lock manager (because PostPrepare_Locks hasn't transfered them to 
the dummy PGPROC yet).


I think we'll need to transfer of the locks earlier, before the 
transaction is marked as fully prepared. I'll take a closer look at this 
tomorrow.


- 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] Problem with txid_snapshot_in/out() functionality

[Jan Wieck]

On 04/13/14 14:22, Jan Wieck wrote:

On 04/13/14 08:27, Marko Kreen wrote:

I think you need to do SET_VARSIZE also here.  Alternative is to
move SET_VARSIZE after sort_snapshot().

And it seems the drop-double-txid logic should be added also to
txid_snapshot_recv().  It seems weird to have it behave differently
from txid_snapshot_in().



Thanks,

yes on both issues. Will create another patch.


New patch attached.

New github commit is 
https://github.com/wieck/postgres/commit/b8fd0d2eb78791e5171e34aecd233fd06218890d



Thanks again,
Jan

--
Jan Wieck
Senior Software Engineer
http://slony.info
diff --git a/src/backend/utils/adt/txid.c b/src/backend/utils/adt/txid.c
index a005e67..1023566 100644
*** a/src/backend/utils/adt/txid.c
--- b/src/backend/utils/adt/txid.c
*** cmp_txid(const void *aa, const void *bb)
*** 131,146 
  }
  
  /*
!  * sort a snapshot's txids, so we can use bsearch() later.
   *
   * For consistency of on-disk representation, we always sort even if bsearch
!  * will not be used.
   */
  static void
  sort_snapshot(TxidSnapshot *snap)
  {
if (snap-nxip  1)
qsort(snap-xip, snap-nxip, sizeof(txid), cmp_txid);
  }
  
  /*
--- 131,161 
  }
  
  /*
!  * unique sort a snapshot's txids, so we can use bsearch() later.
   *
   * For consistency of on-disk representation, we always sort even if bsearch
!  * will not be used. 
   */
  static void
  sort_snapshot(TxidSnapshot *snap)
  {
+   txidlast = 0;
+   int nxip, idx1, idx2;
+ 
if (snap-nxip  1)
+   {
qsort(snap-xip, snap-nxip, sizeof(txid), cmp_txid);
+   nxip = snap-nxip;
+   idx1 = idx2 = 0;
+   while (idx1  nxip)
+   {
+   if (snap-xip[idx1] != last)
+   last = snap-xip[idx2++] = snap-xip[idx1];
+   else
+   snap-nxip--;
+   idx1++;
+   }
+   }
  }
  
  /*
*** parse_snapshot(const char *str)
*** 294,304 
val = str2txid(str, endp);
str = endp;
  
!   /* require the input to be in order */
!   if (val  xmin || val = xmax || val = last_val)
goto bad_format;
  
!   buf_add_txid(buf, val);
last_val = val;
  
if (*str == ',')
--- 309,320 
val = str2txid(str, endp);
str = endp;
  
!   /* require the input to be in order and skip duplicates */
!   if (val  xmin || val = xmax || val  last_val)
goto bad_format;
  
!   if (val != last_val)
!   buf_add_txid(buf, val);
last_val = val;
  
if (*str == ',')
*** txid_current_snapshot(PG_FUNCTION_ARGS)
*** 361,368 
  {
TxidSnapshot *snap;
uint32  nxip,
!   i,
!   size;
TxidEpoch   state;
Snapshotcur;
  
--- 377,383 
  {
TxidSnapshot *snap;
uint32  nxip,
!   i;
TxidEpoch   state;
Snapshotcur;
  
*** txid_current_snapshot(PG_FUNCTION_ARGS)
*** 381,389 
  
/* allocate */
nxip = cur-xcnt;
!   size = TXID_SNAPSHOT_SIZE(nxip);
!   snap = palloc(size);
!   SET_VARSIZE(snap, size);
  
/* fill */
snap-xmin = convert_xid(cur-xmin, state);
--- 396,402 
  
/* allocate */
nxip = cur-xcnt;
!   snap = palloc(TXID_SNAPSHOT_SIZE(nxip));
  
/* fill */
snap-xmin = convert_xid(cur-xmin, state);
*** txid_current_snapshot(PG_FUNCTION_ARGS)
*** 395,400 
--- 408,416 
/* we want them guaranteed to be in ascending order */
sort_snapshot(snap);
  
+   /* we set the size here because the sort may have removed duplicate 
xips */
+   SET_VARSIZE(snap, TXID_SNAPSHOT_SIZE(snap-nxip));
+ 
PG_RETURN_POINTER(snap);
  }
  
*** txid_snapshot_recv(PG_FUNCTION_ARGS)
*** 472,489 
snap = palloc(TXID_SNAPSHOT_SIZE(nxip));
snap-xmin = xmin;
snap-xmax = xmax;
-   snap-nxip = nxip;
-   SET_VARSIZE(snap, TXID_SNAPSHOT_SIZE(nxip));
  
for (i = 0; i  nxip; i++)
{
txidcur = pq_getmsgint64(buf);
  
!   if (cur = last || cur  xmin || cur = xmax)
goto bad_format;
snap-xip[i] = cur;
last = cur;
}
PG_RETURN_POINTER(snap);
  
  bad_format:
--- 488,514 
snap = palloc(TXID_SNAPSHOT_SIZE(nxip));
snap-xmin = xmin;
snap-xmax = xmax;
  
for (i = 0; i  nxip; i++)
{
txidcur = pq_getmsgint64(buf);
  
!   if 

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

[Heikki Linnakangas]

On 04/12/2014 12:07 AM, Jan Wieck wrote:

Hackers,

the Slony team has been getting seldom reports of a problem with the
txid_snapshot data type.

The symptom is that a txid_snapshot on output lists the same txid
multiple times in the xip list part of the external representation. This
string is later rejected by txid_snapshot_in() when trying to determine
if a particular txid is visible in that snapshot using the
txid_visible_in_snapshot() function.

I was not yet able to reproduce this problem in a lab environment. It
might be related to subtransactions and/or two phase commit (at least
one user is using both of them). The reported PostgreSQL version
involved in that case was 9.1.


It's two-phase commit. When preparing a transaction, the state of the 
transaction is first transfered to a dummy PGXACT entry, and then the 
PGXACT entry of the backend is cleared. There is a transient state when 
both PGXACT entries have the same xid.


You can reproduce that by putting a sleep or breakpoint in 
PrepareTransaction(), just before the 
ProcArrayClearTransaction(MyProc); call. If you call 
txid_current_snapshot() from another session at that point, it will 
output two duplicate xids. (you will have to also commit one more 
unrelated transaction to bump up xmax).



At this point I would find it extremely helpful to sanitize the
external representation in txid_snapshot_out() while emitting some
NOTICE level logging when this actually happens. I am aware that this
does amount to a functional change for a back release, but considering
that the _out() generated external representation of an existing binary
datum won't pass the type's _in() function, I argue that such change is
warranted. Especially since this problem could possibly corrupt a dump.


Hmm. Do we snapshots to be stored in tables, and included in a dump? I 
don't think we can guarantee that will work, at least not across 
versions, as the way we handle snapshot internally can change.


But yeah, we probably should do something about that. The most 
straightforward fix would be to scan the array in 
txid_current_snapshot() and remove any duplicates.


- 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] Problem with txid_snapshot_in/out() functionality

[Jan Wieck]

On 04/12/14 03:27, Heikki Linnakangas wrote:

On 04/12/2014 12:07 AM, Jan Wieck wrote:

Hackers,

the Slony team has been getting seldom reports of a problem with the
txid_snapshot data type.

The symptom is that a txid_snapshot on output lists the same txid
multiple times in the xip list part of the external representation. This
string is later rejected by txid_snapshot_in() when trying to determine
if a particular txid is visible in that snapshot using the
txid_visible_in_snapshot() function.

I was not yet able to reproduce this problem in a lab environment. It
might be related to subtransactions and/or two phase commit (at least
one user is using both of them). The reported PostgreSQL version
involved in that case was 9.1.


It's two-phase commit. When preparing a transaction, the state of the
transaction is first transfered to a dummy PGXACT entry, and then the
PGXACT entry of the backend is cleared. There is a transient state when
both PGXACT entries have the same xid.

You can reproduce that by putting a sleep or breakpoint in
PrepareTransaction(), just before the
ProcArrayClearTransaction(MyProc); call. If you call
txid_current_snapshot() from another session at that point, it will
output two duplicate xids. (you will have to also commit one more
unrelated transaction to bump up xmax).


Thanks, that explains it.




At this point I would find it extremely helpful to sanitize the
external representation in txid_snapshot_out() while emitting some
NOTICE level logging when this actually happens. I am aware that this
does amount to a functional change for a back release, but considering
that the _out() generated external representation of an existing binary
datum won't pass the type's _in() function, I argue that such change is
warranted. Especially since this problem could possibly corrupt a dump.


Hmm. Do we snapshots to be stored in tables, and included in a dump? I
don't think we can guarantee that will work, at least not across
versions, as the way we handle snapshot internally can change.


At least Londiste and Slony do store snapshots as well as xids in tables 
and assuming that the txid epoch is properly bumped, that information is 
useful and valid after a restore.




But yeah, we probably should do something about that. The most
straightforward fix would be to scan the array in
txid_current_snapshot() and remove any duplicates.


The code in txid_snapshot_in() checks that the xip list is ascending. 
txid_snapshot_out() does not sort the list, so it must already be sorted 
when the snapshot itself is created. That scan would be fairly simple.



Jan

--
Jan Wieck
Senior Software Engineer
http://slony.info


--
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] Problem with txid_snapshot_in/out() functionality

[Andres Freund]

On 2014-04-12 10:27:16 +0300, Heikki Linnakangas wrote:
 On 04/12/2014 12:07 AM, Jan Wieck wrote:
 The symptom is that a txid_snapshot on output lists the same txid
 multiple times in the xip list part of the external representation. This
 string is later rejected by txid_snapshot_in() when trying to determine
 if a particular txid is visible in that snapshot using the
 txid_visible_in_snapshot() function.

 It's two-phase commit. When preparing a transaction, the state of the
 transaction is first transfered to a dummy PGXACT entry, and then the PGXACT
 entry of the backend is cleared. There is a transient state when both PGXACT
 entries have the same xid.

Which I find to be a pretty bad idea independent of this bug. But I
think that's nothing fixable in the back branches.

 Hmm. Do we snapshots to be stored in tables, and included in a dump? I don't
 think we can guarantee that will work, at least not across versions, as the
 way we handle snapshot internally can change.

Hm. I don't think we'll earn much love changing that - there's at the
very least slony and londiste out there using it... IIRC both store the
result in tables.

 But yeah, we probably should do something about that. The most
 straightforward fix would be to scan the array in txid_current_snapshot()
 and remove any duplicates.

Since it's sorted there, that should be fairly straightforward. Won't
fix already created and stored datums tho. Maybe _in()/parse_snapshot()
should additionally skip over duplicate values? Looks easy enough.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

[Jan Wieck]

On 04/12/14 08:38, Andres Freund wrote:


Since it's sorted there, that should be fairly straightforward. Won't
fix already created and stored datums tho. Maybe _in()/parse_snapshot()
should additionally skip over duplicate values? Looks easy enough.


There is the sort ... missed that when glancing over the code earlier.

Right, that is easy enough and looks like an acceptable fix for back 
branches too.



Thanks,
Jan

--
Jan Wieck
Senior Software Engineer
http://slony.info


--
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] Problem with txid_snapshot_in/out() functionality

[Tom Lane]

Heikki Linnakangas hlinnakan...@vmware.com writes:
 On 04/12/2014 12:07 AM, Jan Wieck wrote:
 the Slony team has been getting seldom reports of a problem with the
 txid_snapshot data type.
 The symptom is that a txid_snapshot on output lists the same txid
 multiple times in the xip list part of the external representation.

 It's two-phase commit. When preparing a transaction, the state of the 
 transaction is first transfered to a dummy PGXACT entry, and then the 
 PGXACT entry of the backend is cleared. There is a transient state when 
 both PGXACT entries have the same xid.

Hm, yeah, but why is that intermediate state visible to anyone else?
Don't we have exclusive lock on the PGPROC array while we're doing this?
If we don't, aren't we letting other backends see non-self-consistent
state in regards to who holds which locks, for example?

I'm worried that the proposed fix is just band-aiding one particular
symptom of inadequate locking.

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] Problem with txid_snapshot_in/out() functionality

[Andres Freund]

On 2014-04-12 09:47:24 -0400, Tom Lane wrote:
 Heikki Linnakangas hlinnakan...@vmware.com writes:
  On 04/12/2014 12:07 AM, Jan Wieck wrote:
  the Slony team has been getting seldom reports of a problem with the
  txid_snapshot data type.
  The symptom is that a txid_snapshot on output lists the same txid
  multiple times in the xip list part of the external representation.

  It's two-phase commit. When preparing a transaction, the state of the
  transaction is first transfered to a dummy PGXACT entry, and then the
  PGXACT entry of the backend is cleared. There is a transient state when
  both PGXACT entries have the same xid.

 Hm, yeah, but why is that intermediate state visible to anyone else?
 Don't we have exclusive lock on the PGPROC array while we're doing this?

It's done outside the remit of ProcArray lock :(. And documented to lead
to duplicate xids in PGXACT.
EndPrepare():
/*
 * Mark the prepared transaction as valid.  As soon as xact.c marks
 * MyPgXact as not running our XID (which it will do immediately after
 * this function returns), others can commit/rollback the xact.
 *
 * NB: a side effect of this is to make a dummy ProcArray entry for the
 * prepared XID.  This must happen before we clear the XID from 
MyPgXact,
 * else there is a window where the XID is not running according to
 * TransactionIdIsInProgress, and onlookers would be entitled to assume
 * the xact crashed.  Instead we have a window where the same XID 
appears
 * twice in ProcArray, which is OK.
 */
MarkAsPrepared(gxact);

It doesn't sound too hard to essentially move PrepareTransaction()'s
ProcArrayClearTransaction() into MarkAsPrepared() and rejigger the
locking to remove the intermediate state. But I think it'll lead to
bigger changes than we'd be comfortable backpatching.

 If we don't, aren't we letting other backends see non-self-consistent
 state in regards to who holds which locks, for example?

I think that actually works out ok, because the locks aren't owned by
xids/xacts, but procs. Otherwise we'd be in deep trouble in
CommitTransaction() as well where ProcArrayEndTransaction() clearing
that state.
After the whole xid transfer, there's PostPrepare_Locks() transferring
the locks.

Brr.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

[Greg Stark]

On 12 Apr 2014 08:35, Jan Wieck j...@wi3ck.info wrote:

 On 04/12/14 03:27, Heikki Linnakangas wrote:

 On 04/12/2014 12:07 AM, Jan Wieck wrote:

 Hackers,

 Hmm. Do we snapshots to be stored in tables, and included in a dump? I
 don't think we can guarantee that will work, at least not across
 versions, as the way we handle snapshot internally can change.


 At least Londiste and Slony do store snapshots as well as xids in tables
and assuming that the txid epoch is properly bumped, that information is
useful and valid after a restore.

As I understand it the epoch increments whenever the xid wraps.

A physical restore would continue the same xid space in the same epoch
which should work fine as long as no system stores any txids outside the
database from the future.

A pg_restore would start a new xid space from FirstNormalXid which would
obviously not work with any stored txids.

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

[Jan Wieck]

On 04/12/14 10:09, Greg Stark wrote:


A pg_restore would start a new xid space from FirstNormalXid which would
obviously not work with any stored txids.



http://www.postgresql.org/docs/9.3/static/app-pgresetxlog.html


Regards,
Jan

--
Jan Wieck
Senior Software Engineer
http://slony.info


--
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] Problem with txid_snapshot_in/out() functionality

[Andres Freund]

On 2014-04-12 11:15:09 -0400, Jan Wieck wrote:
 On 04/12/14 10:09, Greg Stark wrote:
 
 A pg_restore would start a new xid space from FirstNormalXid which would
 obviously not work with any stored txids.
 
 
 http://www.postgresql.org/docs/9.3/static/app-pgresetxlog.html

Using that as part of any sort of routine task IMNSHO is a seriously bad
idea.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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

Re: [HACKERS] Problem with txid_snapshot_in/out() functionality

[Jan Wieck]

On 04/12/14 11:18, Andres Freund wrote:

On 2014-04-12 11:15:09 -0400, Jan Wieck wrote:

On 04/12/14 10:09, Greg Stark wrote:

A pg_restore would start a new xid space from FirstNormalXid which would
obviously not work with any stored txids.


http://www.postgresql.org/docs/9.3/static/app-pgresetxlog.html


Using that as part of any sort of routine task IMNSHO is a seriously bad
idea.


Nobody is advocating doing so.


Jan

--
Jan Wieck
Senior Software Engineer
http://slony.info


--
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] Problem with txid_snapshot_in/out() functionality

[Jan Wieck]

On 04/12/14 10:03, Andres Freund wrote:

On 2014-04-12 09:47:24 -0400, Tom Lane wrote:

Heikki Linnakangas hlinnakan...@vmware.com writes:
 On 04/12/2014 12:07 AM, Jan Wieck wrote:
 the Slony team has been getting seldom reports of a problem with the
 txid_snapshot data type.
 The symptom is that a txid_snapshot on output lists the same txid
 multiple times in the xip list part of the external representation.

 It's two-phase commit. When preparing a transaction, the state of the
 transaction is first transfered to a dummy PGXACT entry, and then the
 PGXACT entry of the backend is cleared. There is a transient state when
 both PGXACT entries have the same xid.

Hm, yeah, but why is that intermediate state visible to anyone else?
Don't we have exclusive lock on the PGPROC array while we're doing this?


It's done outside the remit of ProcArray lock :(. And documented to lead
to duplicate xids in PGXACT.
EndPrepare():
/*
 * Mark the prepared transaction as valid.  As soon as xact.c marks
 * MyPgXact as not running our XID (which it will do immediately after
 * this function returns), others can commit/rollback the xact.
 *
 * NB: a side effect of this is to make a dummy ProcArray entry for the
 * prepared XID.  This must happen before we clear the XID from 
MyPgXact,
 * else there is a window where the XID is not running according to
 * TransactionIdIsInProgress, and onlookers would be entitled to assume
 * the xact crashed.  Instead we have a window where the same XID 
appears
 * twice in ProcArray, which is OK.
 */
MarkAsPrepared(gxact);

It doesn't sound too hard to essentially move PrepareTransaction()'s
ProcArrayClearTransaction() into MarkAsPrepared() and rejigger the
locking to remove the intermediate state. But I think it'll lead to
bigger changes than we'd be comfortable backpatching.


If we don't, aren't we letting other backends see non-self-consistent
state in regards to who holds which locks, for example?


I think that actually works out ok, because the locks aren't owned by
xids/xacts, but procs. Otherwise we'd be in deep trouble in
CommitTransaction() as well where ProcArrayEndTransaction() clearing
that state.
After the whole xid transfer, there's PostPrepare_Locks() transferring
the locks.


Since it doesn't seem to produce any side effects, I'd think that making 
the snapshot unique within txid_current_snapshot() and filtering 
duplicates on input should be sufficient and eligible for backpatching.


The attached patch adds a unique loop to the internal sort_snapshot() 
function and skips duplicates on input. The git commit is here:


https://github.com/wieck/postgres/commit/a88a2b2c25b856478d7e2b012fc718106338fe00


Regards,
Jan

--
Jan Wieck
Senior Software Engineer
http://slony.info
diff --git a/src/backend/utils/adt/txid.c b/src/backend/utils/adt/txid.c
index a005e67..44f7880 100644
*** a/src/backend/utils/adt/txid.c
--- b/src/backend/utils/adt/txid.c
*** cmp_txid(const void *aa, const void *bb)
*** 131,146 
  }
  
  /*
!  * sort a snapshot's txids, so we can use bsearch() later.
   *
   * For consistency of on-disk representation, we always sort even if bsearch
!  * will not be used.
   */
  static void
  sort_snapshot(TxidSnapshot *snap)
  {
if (snap-nxip  1)
qsort(snap-xip, snap-nxip, sizeof(txid), cmp_txid);
  }
  
  /*
--- 131,161 
  }
  
  /*
!  * unique sort a snapshot's txids, so we can use bsearch() later.
   *
   * For consistency of on-disk representation, we always sort even if bsearch
!  * will not be used. 
   */
  static void
  sort_snapshot(TxidSnapshot *snap)
  {
+   txidlast = 0;
+   int nxip, idx1, idx2;
+ 
if (snap-nxip  1)
+   {
qsort(snap-xip, snap-nxip, sizeof(txid), cmp_txid);
+   nxip = snap-nxip;
+   idx1 = idx2 = 0;
+   while (idx1  nxip)
+   {
+   if (snap-xip[idx1] != last)
+   last = snap-xip[idx2++] = snap-xip[idx1];
+   else
+   snap-nxip--;
+   idx1++;
+   }
+   }
  }
  
  /*
*** parse_snapshot(const char *str)
*** 294,304 
val = str2txid(str, endp);
str = endp;
  
!   /* require the input to be in order */
!   if (val  xmin || val = xmax || val = last_val)
goto bad_format;
  
!   buf_add_txid(buf, val);
last_val = val;
  
if (*str == ',')
--- 309,320 
val = str2txid(str, endp);
str = endp;
  
!   /* require the input to be in order and skip duplicates */
!   if (val  xmin || val = xmax || val  last_val)
goto bad_format;
  
!   if (val != 

Re: [HACKERS] Problem with displaying wide tables in psql

[Sergey Muraviov]

Hi.

I've done some corrections for printing newline and wrap indicators.
Please review the attached patch.


2014-04-11 0:14 GMT+04:00 Sergey Muraviov sergey.k.murav...@gmail.com:

 Hi.

 Thanks for your tests.

 I've fixed problem with headers, but got new one with data.
 I'll try to solve it tomorrow.


 2014-04-10 18:45 GMT+04:00 Greg Stark st...@mit.edu:

 Ok, So I've hacked on this a bit. Below is a test case showing the
 problems I've found.

 1) It isn't using the newline and wrap indicators or dividing lines.

 2) The header is not being displayed properly when it contains a newline.

 I can hack in the newline and wrap indicators but the header
 formatting requires reworking the logic a bit. The header and data
 need to be stepped through in parallel rather than having a loop to
 handle the wrapping within the handling of a single line. I don't
 really have time for that today but if you can get to it that would be
 fine,




 --
 Best regards,
 Sergey Muraviov




-- 
Best regards,
Sergey Muraviov
From 5e0f44994d04a81523920a78d3a35603e919170c Mon Sep 17 00:00:00 2001
From: Sergey Muraviov sergey.k.murav...@gmail.com
Date: Fri, 11 Apr 2014 11:03:41 +0400
Subject: [PATCH] Using newline and wrap indicators

---
 src/bin/psql/print.c | 130 +++
 1 file changed, 110 insertions(+), 20 deletions(-)

diff --git a/src/bin/psql/print.c b/src/bin/psql/print.c
index 79fc43e..6463162 100644
--- a/src/bin/psql/print.c
+++ b/src/bin/psql/print.c
@@ -1234,13 +1234,56 @@ print_aligned_vertical(const printTableContent *cont, FILE *fout)
 			fprintf(fout, %s\n, cont-title);
 	}
 
+		if (cont-opt-format == PRINT_WRAPPED)
+	{
+		int output_columns = 0;
+
+		/*
+		 * Choose target output width: \pset columns, or $COLUMNS, or ioctl
+		 */
+		if (cont-opt-columns  0)
+			output_columns = cont-opt-columns;
+		else
+		{
+			if (cont-opt-env_columns  0)
+output_columns = cont-opt-env_columns;
+#ifdef TIOCGWINSZ
+			else
+			{
+struct winsize screen_size;
+
+if (ioctl(fileno(stdout), TIOCGWINSZ, screen_size) != -1)
+	output_columns = screen_size.ws_col;
+			}
+#endif
+		}
+
+		output_columns -= hwidth;
+
+		if (opt_border == 0)
+			output_columns -= 1;
+		else
+		{
+			output_columns -= 3; /* -+- */
+
+			if (opt_border  1)
+output_columns -= 4; /* +--+ */
+		}
+
+		if (output_columns  0  dwidth  output_columns)
+			dwidth = output_columns;
+	}
+
 	/* print records */
 	for (i = 0, ptr = cont-cells; *ptr; i++, ptr++)
 	{
 		printTextRule pos;
-		int			line_count,
+		int			dline,
+	hline,
 	dcomplete,
-	hcomplete;
+	hcomplete,
+	offset,
+	chars_to_output;
 
 		if (cancel_pressed)
 			break;
@@ -1270,48 +1313,95 @@ print_aligned_vertical(const printTableContent *cont, FILE *fout)
 		pg_wcsformat((const unsigned char *) *ptr, strlen(*ptr), encoding,
 	 dlineptr, dheight);
 
-		line_count = 0;
+		dline = hline = 0;
 		dcomplete = hcomplete = 0;
+		offset = 0;
+		chars_to_output = dlineptr[dline].width;
 		while (!dcomplete || !hcomplete)
 		{
+			/* Left border */
 			if (opt_border == 2)
 fprintf(fout, %s , dformat-leftvrule);
+
+			/* Header */
 			if (!hcomplete)
 			{
-fprintf(fout, %-s%*s, hlineptr[line_count].ptr,
-		hwidth - hlineptr[line_count].width, );
+int swidth = hwidth - hlineptr[hline].width - 1;
+fprintf(fout, %-s, hlineptr[hline].ptr);
+if (swidth  0) /* spacer */
+	fprintf(fout, %*s, swidth, );
 
-if (!hlineptr[line_count + 1].ptr)
+if (!hlineptr[hline + 1].ptr)
+{
+	fputs( , fout);
 	hcomplete = 1;
+}
+else 
+{
+	fputs(format-nl_right, fout);
+	hline++;
+}
 			}
 			else
-fprintf(fout, %*s, hwidth, );
+fprintf(fout, %*s, hwidth + 1, );
 
+			/* Separator */
 			if (opt_border  0)
-fprintf(fout,  %s , dformat-midvrule);
-			else
-fputc(' ', fout);
+fprintf(fout, %s, dformat-midvrule);
 
+			/* Data */
 			if (!dcomplete)
 			{
-if (opt_border  2)
-	fprintf(fout, %s\n, dlineptr[line_count].ptr);
+int target_width,
+	bytes_to_output,
+	swidth;
+
+fputs(!dcomplete  !offset?  : format-wrap_left, fout);
+
+target_width = dwidth;
+bytes_to_output = strlen_max_width(dlineptr[dline].ptr + offset,
+   target_width, encoding);
+fputnbytes(fout, (char *)(dlineptr[dline].ptr + offset),
+		   bytes_to_output);
+
+chars_to_output -= target_width;
+offset += bytes_to_output;
+
+/* spacer */
+swidth = dwidth - target_width;
+if (swidth  0)
+	fprintf(fout, %*s, swidth, );
+
+if (!chars_to_output)
+{
+	if (!dlineptr[dline + 1].ptr)
+	{
+		fputs( , fout);
+		dcomplete = 1;
+	}
+	else
+	{
+		fputs(format-nl_right, fout);
+		dline++;
+		offset = 0;
+		chars_to_output = dlineptr[dline].width;
+	}
+}
 else
-	fprintf(fout, %-s%*s %s\n, dlineptr[line_count].ptr,
-			dwidth - dlineptr[line_count].width, ,

Re: [HACKERS] Problem with displaying wide tables in psql

[Greg Stark]

Looks good.

It's still not doing the old-ascii column dividers but to be honest
I'm not sure what the intended behaviour of old-ascii is. I've noticed
old-ascii only displays the line markers for dividing lines, not the
final one. That seems pretty useless and maybe it's why we switched to
the new ascii mode, I don't recall.

This is the way old-ascii displays when two columns are wrapping -- it
seems pretty confused to me. The headers are displaying newline
markers from the ascii style instead of : indicators in the dividing
line. The : and ; markers are only displayed for the first column, not
the second one.

Maybe since the : and ; markers aren't shown for the final column that
means the expanded mode doesn't have to do anything since the column
is only ever the final column.


++-+
| x  |x|
|+y  |+   y|
|+z  |+   z|
++-+
| x  | xxx |
| xx ; |
| xxx: xxx |
|    ; xxx |
| x  : xxx |
| xx ; xx  |
| xxx: xxx |
|    ; x   |
| x  : xxx |
| xx : xx  |
| xxx: x   |
|    : |
| x  : xxx |
| xx : xx  |
| xxx: x   |
|    : |
| x  : xxx |
| xx : xx  |
| xx : x   |
| x  : |
| xx : xxx |
| xx : xx  |
| xx : x   |
| xxx: |
| xx : |
|    : |
| xx : |
| x  : |
| xx : |
| xx   |
| xx   |
| xxx  |
++-+
stark=# select array_to_string(array_agg(repeat('x',x)),E'\n') as x
y
z
from generate_series(1,20) as x(x);

┌─[ RECORD 1 ]─┐
│ x↵│ x   ↵│
│ y↵│ xx  ↵│
│ z │ xxx ↵│
│   │ ↵│
│   │ x   ↵│
│   │ xx  ↵│
│   │ xxx ↵│
│   │ ↵│
│   │ x   ↵│
│   │ xx  ↵│
│   │ xxx ↵│
│   │ ↵│
│   │ …│
│   │…x   ↵│
│   │ …│
│   │…xx  ↵│
│   │ …│
│   │…xxx ↵│
│   │ …│
│   │…↵│
│   │ …│
│   │…x   ↵│
│   │ …│
│   │…xx  ↵│
│   │ …│
│   │…xxx ↵│
│   │ …│
│   │… │
└───┴──┘

stark=# \pset linestyle ascii
Line style (linestyle) is ascii.

stark=# select array_to_string(array_agg(repeat('x',x)),E'\n') as x
y
z
from generate_series(1,20) as x(x);
stark***# stark***# stark-***# +-[ RECORD 1 ]-+
| x+| x   +|
| y+| xx  +|
| z | xxx +|
|   | +|
|   | x   +|
|   | xx  +|
|   | xxx +|
|   | +|
|   | x   +|
|   | xx  +|
|   | xxx +|
|   | +|
|   | .|
|   |.x   +|
|   | .|
|   |.xx  +|
|   | .|
|   |.xxx +|
|   | .|
|   |.+|
|   | .|
|   |.x   +|
|   | .|
|   |.xx  +|
|   | .|
|   |.xxx +|
|   | .|
|   |. |
+---+--+

stark=# \pset linestyle old-ascii
Line style (linestyle) is old-ascii.

stark=# select array_to_string(array_agg(repeat('x',x)),E'\n') as x
y
z
from generate_series(1,20) as x(x);
stark# stark# stark-# 
+-[ RECORD 1 ]-+
| x | x|
| y | xx   |
| z | xxx  |
|   |  |
|   | x|
|   | xx   |
|   | xxx  |
|   |  |
|   | x|
|   | xx   |
|   | xxx  |
|   |  |
|   |  |
|   | x|
|   |  |
|   | xx   |
|   |  |
|   | xxx  |
|   |  |
|   |  |
|   |  |
|   | x|
|   |  |
|   | xx   |
|   |  |
|   | xxx  |
|   |  |
|   |  |
+---+--+

stark=# \pset expanded off
Expanded display (expanded) is off.

stark=# \pset columns 40
Target width (columns) is 40.

stark=# select array_to_string(array_agg(repeat('x',x)),E'\n') as x
y

Re: [HACKERS] Problem with displaying wide tables in psql

[Sergey Muraviov]

I hope that I realized old-ascii logic correctly.


2014-04-11 19:10 GMT+04:00 Greg Stark st...@mit.edu:

 Looks good.

 It's still not doing the old-ascii column dividers but to be honest
 I'm not sure what the intended behaviour of old-ascii is. I've noticed
 old-ascii only displays the line markers for dividing lines, not the
 final one. That seems pretty useless and maybe it's why we switched to
 the new ascii mode, I don't recall.

 This is the way old-ascii displays when two columns are wrapping -- it
 seems pretty confused to me. The headers are displaying newline
 markers from the ascii style instead of : indicators in the dividing
 line. The : and ; markers are only displayed for the first column, not
 the second one.

 Maybe since the : and ; markers aren't shown for the final column that
 means the expanded mode doesn't have to do anything since the column
 is only ever the final column.


 ++-+
 | x  |x|
 |+y  |+   y|
 |+z  |+   z|
 ++-+
 | x  | xxx |
 | xx ; |
 | xxx: xxx |
 |    ; xxx |
 | x  : xxx |
 | xx ; xx  |
 | xxx: xxx |
 |    ; x   |
 | x  : xxx |
 | xx : xx  |
 | xxx: x   |
 |    : |
 | x  : xxx |
 | xx : xx  |
 | xxx: x   |
 |    : |
 | x  : xxx |
 | xx : xx  |
 | xx : x   |
 | x  : |
 | xx : xxx |
 | xx : xx  |
 | xx : x   |
 | xxx: |
 | xx : |
 |    : |
 | xx : |
 | x  : |
 | xx : |
 | xx   |
 | xx   |
 | xxx  |
 ++-+




-- 
Best regards,
Sergey Muraviov
From 69d845f05864a5d07a90ee20e10a5cb09b78d1d3 Mon Sep 17 00:00:00 2001
From: Sergey Muraviov sergey.k.murav...@gmail.com
Date: Fri, 11 Apr 2014 20:55:17 +0400
Subject: [PATCH] Support for old-ascii mode

---
 src/bin/psql/print.c | 144 +++
 1 file changed, 122 insertions(+), 22 deletions(-)

diff --git a/src/bin/psql/print.c b/src/bin/psql/print.c
index 79fc43e..2c58cb5 100644
--- a/src/bin/psql/print.c
+++ b/src/bin/psql/print.c
@@ -1234,13 +1234,56 @@ print_aligned_vertical(const printTableContent *cont, FILE *fout)
 			fprintf(fout, %s\n, cont-title);
 	}
 
+	if (cont-opt-format == PRINT_WRAPPED)
+	{
+		int output_columns = 0;
+
+		/*
+		 * Choose target output width: \pset columns, or $COLUMNS, or ioctl
+		 */
+		if (cont-opt-columns  0)
+			output_columns = cont-opt-columns;
+		else
+		{
+			if (cont-opt-env_columns  0)
+output_columns = cont-opt-env_columns;
+#ifdef TIOCGWINSZ
+			else
+			{
+struct winsize screen_size;
+
+if (ioctl(fileno(stdout), TIOCGWINSZ, screen_size) != -1)
+	output_columns = screen_size.ws_col;
+			}
+#endif
+		}
+
+		output_columns -= hwidth;
+
+		if (opt_border == 0)
+			output_columns -= 1;
+		else
+		{
+			output_columns -= 3; /* -+- */
+
+			if (opt_border  1)
+output_columns -= 4; /* +--+ */
+		}
+
+		if (output_columns  0  dwidth  output_columns)
+			dwidth = output_columns;
+	}
+
 	/* print records */
 	for (i = 0, ptr = cont-cells; *ptr; i++, ptr++)
 	{
 		printTextRule pos;
-		int			line_count,
+		int			dline,
+	hline,
 	dcomplete,
-	hcomplete;
+	hcomplete,
+	offset,
+	chars_to_output;
 
 		if (cancel_pressed)
 			break;
@@ -1270,48 +1313,105 @@ print_aligned_vertical(const printTableContent *cont, FILE *fout)
 		pg_wcsformat((const unsigned char *) *ptr, strlen(*ptr), encoding,
 	 dlineptr, dheight);
 
-		line_count = 0;
+		dline = hline = 0;
 		dcomplete = hcomplete = 0;
+		offset = 0;
+		chars_to_output = dlineptr[dline].width;
 		while (!dcomplete || !hcomplete)
 		{
+			/* Left border */
 			if (opt_border == 2)
-fprintf(fout, %s , dformat-leftvrule);
+fputs(dformat-leftvrule, fout);
+
+			/* Header */
 			if (!hcomplete)
 			{
-fprintf(fout, %-s%*s, hlineptr[line_count].ptr,
-		hwidth - hlineptr[line_count].width, );
+int swidth = hwidth - hlineptr[hline].width - 1;
+fputs(hline? format-header_nl_left:  , fout);
+fprintf(fout, %-s, 

Re: [HACKERS] Problem with displaying wide tables in psql

[Alvaro Herrera]

Sergey Muraviov wrote:
 I hope that I realized old-ascii logic correctly.

I don't know what you changed here, but I don't think we need to
preserve old-ascii behavior in the new code, in any way.  If you're
mimicking something obsolete and the end result of the new feature is
not great, perhaps that should be rethought.

Can you please provide sample output for the previous version compared
to this new version?  What changed?


-- 
Álvaro Herrerahttp://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] Problem with displaying wide tables in psql

[Sergey Muraviov]

There were no support for wrapping and old-ascii line style in expanded
mode at all.
But now they are.



2014-04-11 21:58 GMT+04:00 Alvaro Herrera alvhe...@2ndquadrant.com:

 Sergey Muraviov wrote:
  I hope that I realized old-ascii logic correctly.

 I don't know what you changed here, but I don't think we need to
 preserve old-ascii behavior in the new code, in any way.  If you're
 mimicking something obsolete and the end result of the new feature is
 not great, perhaps that should be rethought.

 Can you please provide sample output for the previous version compared
 to this new version?  What changed?


 --
 Álvaro Herrerahttp://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services




-- 
Best regards,
Sergey Muraviov
\pset expanded on
\pset border 2
\pset columns 20
\pset format wrapped

\pset linestyle unicode 
postgres=# select array_to_string(array_agg(repeat('x',n)),E'\n') as x
postgres# y,  array_to_string(array_agg(repeat('yy',10-n)),E'\n') as x
postgres# y
postgres# z from (select generate_series(1,10)) as t(n);
┌─[ RECORD 1 ]─┐
│ x↵│ x   ↵│
│ y │ xx  ↵│
│   │ xxx ↵│
│   │ ↵│
│   │ x   ↵│
│   │ xx  ↵│
│   │ xxx ↵│
│   │ ↵│
│   │ x   ↵│
│   │ xx   │
│ x↵│ …│
│ y↵│…yy  ↵│
│ z │ …│
│   │…↵│
│   │ …│
│   │…yy  ↵│
│   │ ↵│
│   │ yy  ↵│
│   │ ↵│
│   │ yy  ↵│
│   │ ↵│
│   │ yy  ↵│
│   │  │
└───┴──┘

postgres=# \pset linestyle ascii 
Line style (linestyle) is ascii.
postgres=# select array_to_string(array_agg(repeat('x',n)),E'\n') as x
y,  array_to_string(array_agg(repeat('yy',10-n)),E'\n') as x
y
z from (select generate_series(1,10)) as t(n);
+-[ RECORD 1 ]-+
| x+| x   +|
| y | xx  +|
|   | xxx +|
|   | +|
|   | x   +|
|   | xx  +|
|   | xxx +|
|   | +|
|   | x   +|
|   | xx   |
| x+| .|
| y+|.yy  +|
| z | .|
|   |.+|
|   | .|
|   |.yy  +|
|   | +|
|   | yy  +|
|   | +|
|   | yy  +|
|   | +|
|   | yy  +|
|   |  |
+---+--+

postgres=# \pset linestyle old-ascii 
Line style (linestyle) is old-ascii.
postgres=# select array_to_string(array_agg(repeat('x',n)),E'\n') as x
y,  array_to_string(array_agg(repeat('yy',10-n)),E'\n') as x
y
z from (select generate_series(1,10)) as t(n);
+-[ RECORD 1 ]-+
| x | x|
|+y : xx   |
|   : xxx  |
|   :  |
|   : x|
|   : xx   |
|   : xxx  |
|   :  |
|   : x|
|   : xx   |
| x |  |
|+y ; yy   |
|+z :  |
|   ;  |
|   :  |
|   ; yy   |
|   :  |
|   : yy   |
|   :  |
|   : yy   |
|   :  |
|   : yy   |
|   :  |
+---+--+



-- 
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] Problem with displaying wide tables in psql

[Greg Stark]

Yeah, I think I agree. I'm pretty happy to commit it without old-ascii
doing anything special.

It looks to me like old-ascii just isn't very useful for a single column
output (like expanded mode is implicitly). Maybe that needs to be fixed but
then it needs to be fixed for non expanded mode as well.

I don't think this new output is very useful dive it just displays the
old-ascii info for the header cells. The header cells never wrap and often
have a lot of empty lines so it just looks like noise.

I do think the earlier change today was important. It looks great now with
the wrap and newline indicators in ascii and Unicode mode.

Fwiw I've switched my .psqlrc to use Unicode and wrapped=auto be default.
It makes psql way more usable. It's even better than my old technique of
running it in Emacs and scrolling left and right.

I'll take a look at it this evening and will add regression tests (and
probably more comments too!) and commit. I want to try adding Unicode
regression tests but I'm not sure what the state of the art is for Unicode
fallback behaviour on the build farm. I think there are some existing tests
we can copy iirc.

-- 
greg

[HACKERS] Problem with txid_snapshot_in/out() functionality

[Jan Wieck]

Hackers,

the Slony team has been getting seldom reports of a problem with the 
txid_snapshot data type.


The symptom is that a txid_snapshot on output lists the same txid 
multiple times in the xip list part of the external representation. This 
string is later rejected by txid_snapshot_in() when trying to determine 
if a particular txid is visible in that snapshot using the 
txid_visible_in_snapshot() function.


I was not yet able to reproduce this problem in a lab environment. It 
might be related to subtransactions and/or two phase commit (at least 
one user is using both of them). The reported PostgreSQL version 
involved in that case was 9.1.


At this point I would find it extremely helpful to sanitize the 
external representation in txid_snapshot_out() while emitting some 
NOTICE level logging when this actually happens. I am aware that this 
does amount to a functional change for a back release, but considering 
that the _out() generated external representation of an existing binary 
datum won't pass the type's _in() function, I argue that such change is 
warranted. Especially since this problem could possibly corrupt a dump.



Comments?


Jan

--
Jan Wieck
Senior Software Engineer
http://slony.info


--
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] Problem with displaying wide tables in psql

[Greg Stark]

Ok, So I've hacked on this a bit. Below is a test case showing the
problems I've found.

1) It isn't using the newline and wrap indicators or dividing lines.

2) The header is not being displayed properly when it contains a newline.

I can hack in the newline and wrap indicators but the header
formatting requires reworking the logic a bit. The header and data
need to be stepped through in parallel rather than having a loop to
handle the wrapping within the handling of a single line. I don't
really have time for that today but if you can get to it that would be
fine,
postgres=***# \pset
Border style (border) is 2.
Target width (columns) is 20.
Expanded display (expanded) is on.
Field separator (fieldsep) is |.
Default footer (footer) is on.
Output format (format) is wrapped.
Line style (linestyle) is unicode.
Null display (null) is .
Locale-adjusted numeric output (numericlocale) is off.
Pager (pager) usage is off.
Record separator (recordsep) is newline.
Table attributes (tableattr) unset.
Title (title) unset.
Tuples only (tuples_only) is off.

postgres=***# select array_to_string(array_agg(repeat('x',n)),E'\n') as x
y,  array_to_string(array_agg(repeat('yy',10-n)),E'\n') as x
y
z from (select generate_series(1,10)) as t(n);

┌─[ RECORD 1 ]─┐
│ x │ x│
│ y │ xx   │
│   │ xxx  │
│   │  │
│   │ x│
│   │ xx   │
│   │ xxx  │
│   │  │
│   │ x│
│   │ xx   │
│ x │  │
│   │ yy   │
│ y │  │
│   │  │
│ z │  │
│   │ yy   │
│   │  │
│   │ yy   │
│   │  │
│   │ yy   │
│   │  │
│   │ yy   │
│   │  │
└───┴──┘

postgres=***# \pset linestyle ascii
Line style (linestyle) is ascii.

postgres=***# select array_to_string(array_agg(repeat('x',n)),E'\n') as x
y,  array_to_string(array_agg(repeat('yy',10-n)),E'\n') as x
y
z from (select generate_series(1,10)) as t(n);

+-[ RECORD 1 ]-+
| x | x|
| y | xx   |
|   | xxx  |
|   |  |
|   | x|
|   | xx   |
|   | xxx  |
|   |  |
|   | x|
|   | xx   |
| x |  |
|   | yy   |
| y |  |
|   |  |
| z |  |
|   | yy   |
|   |  |
|   | yy   |
|   |  |
|   | yy   |
|   |  |
|   | yy   |
|   |  |
+---+--+

postgres=***# \pset columns 30
Target width (columns) is 30.

postgres=***# \pset linestyle unicode
Line style (linestyle) is unicode.

postgres=***# \pset columns 30
Target width (columns) is 30.

postgres=***# \pset expanded off
Expanded display (expanded) is off.

postgres=***# select array_to_string(array_agg(repeat('x',n)),E'\n') as x
y,  array_to_string(array_agg(repeat('yy',10-n)),E'\n') as x
y
z from (select generate_series(1,10)) as t(n);

┌───┬┐
│ x↵│   x   ↵│
│ y │   y   ↵│
│   │   z│
├───┼┤
│ x↵│ yy…│
│ xx   ↵│…  ↵│
│ xxx  ↵│ yy…│
│  ↵│…yy↵│
│ x↵│ yy↵│
│ xx   ↵│   ↵│
│ xxx  ↵│ yy↵│
│  ↵│   ↵│
│ x↵│ yy↵│
│ x…│   ↵│
│…x │ yy↵│
│   ││
└───┴┘
(1 row)



postgres=***# \pset linestyle ascii
Line style (linestyle) is ascii.

postgres=***# select array_to_string(array_agg(repeat('x',n)),E'\n') as x
y,  array_to_string(array_agg(repeat('yy',10-n)),E'\n') as x
y
z from (select generate_series(1,10)) as t(n);

+---++
| x+|   x   +|
| y |   y   +|
|   |   z|
+---++
| x+| yy.|
| xx   +|.  +|
| xxx  +| yy.|
|  +|.yy+|
| x+| yy+|
| xx   +|   +|
| xxx  +| yy+|
|  +|   +|
| x+| yy+|
| x.|   +|
|.x | yy+|
|   ||
+---++

-- 
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] Problem with displaying wide tables in psql

[Sergey Muraviov]

Hi.

Thanks for your tests.

I've fixed problem with headers, but got new one with data.
I'll try to solve it tomorrow.


2014-04-10 18:45 GMT+04:00 Greg Stark st...@mit.edu:

 Ok, So I've hacked on this a bit. Below is a test case showing the
 problems I've found.

 1) It isn't using the newline and wrap indicators or dividing lines.

 2) The header is not being displayed properly when it contains a newline.

 I can hack in the newline and wrap indicators but the header
 formatting requires reworking the logic a bit. The header and data
 need to be stepped through in parallel rather than having a loop to
 handle the wrapping within the handling of a single line. I don't
 really have time for that today but if you can get to it that would be
 fine,




-- 
Best regards,
Sergey Muraviov

Re: [HACKERS] Problem with displaying wide tables in psql

[Sergey Muraviov]

Hi.

How can I pass or set the value of pset variable for an regression test?

The code with ioctl was copied from print_aligned_text function, which has
a long history.
43ee2282 (Bruce Momjian  2008-05-16 16:59:05 +  680)
 if (ioctl(fileno(stdout), TIOCGWINSZ, screen_size) != -1)

2014-04-08 20:27 GMT+04:00 Greg Stark st...@mit.edu:

 On Tue, Apr 8, 2014 at 12:19 PM, Andres Freund and...@2ndquadrant.com
 wrote:
  I don't think this is easily testable that way - doesn't it rely on
  determining the width of the terminal? Which you won't have when started
  from pg_regress?

 There's a pset variable to set the target width so at least the
 formatting code can be tested. It would be nice to have the ioctl at
 least get called on the regression farm so we're sure we aren't doing
 something unportable.

 --
 greg




-- 
Best regards,
Sergey Muraviov

Re: [HACKERS] Problem with displaying wide tables in psql

[Greg Stark]

 How can I pass or set the value of pset variable for an regression test?

I just wrote some tests using \pset columns to control the output.

Having figured out what the point of the patch is I'm pretty happy
with the functionality. It definitely is something I would appreciate
having.

One thing I noticed is that it's not using the formatting characters
to indicate newlines and wrapping. I'm trying to add that myself but
it's taking me a bit to figure out what's going on in the formatting
code. I do have a feeling this is all rearranging the deck chairs on
an obsolete technology and this should all be replaced with a web ui.


-- 
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] Problem with displaying wide tables in psql

[Greg Stark]

On Sat, Feb 15, 2014 at 11:08 AM, Emre Hasegeli e...@hasegeli.com wrote:
 This is my review about 3th version of the patch. It is an useful
 improvement in my opinion. It worked well on my environment.


I'm reviewing this patch.

One thing to comment:

With no doc changes and no regression tests I was halfway inclined to
just reject it out of hand. To be fair there were no regression tests
for wrapped output prior to the patch but still I would have wanted to
see them added. We often pare down regression tests when committing
patches but it's easier to pare them down than write new ones and it
helps show the author's intention.

In this case I'm inclined to expand the regression tests. We've had
bugs in these formatting functions before and at least I find it hard
to touch code like this with any confidence that I'm not breaking
things. Formatting code ends up being pretty spaghetti-like easily and
there are lots of cases so it's easy to unintentionally break cases
you didn't realize you were touching.

In addition there are several cases of logic that looks like this:

if (x)
  ..
else
   {
 if (y)
   ...
 else
   ...
   }

I know there are other opinions on this but I find this logic very
difficut to follow. It's almost always clearer to refactor the
branches into a flat if / else if / else if /.../ else form. Even if
it results in some duplication of code (typically there's some trivial
bit of code outside the second if)  it's easy to quickly see whether
all the cases are handled and understand whether any of the cases have
forgotten any steps.

-- 
greg


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

Re: [HACKERS] Problem with displaying wide tables in psql

[Andres Freund]

On 2014-04-08 12:15:47 -0400, Greg Stark wrote:
 With no doc changes and no regression tests I was halfway inclined to
 just reject it out of hand. To be fair there were no regression tests
 for wrapped output prior to the patch but still I would have wanted to
 see them added. We often pare down regression tests when committing
 patches but it's easier to pare them down than write new ones and it
 helps show the author's intention.

I don't think this is easily testable that way - doesn't it rely on
determining the width of the terminal? Which you won't have when started
from pg_regress?

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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

Re: [HACKERS] Problem with displaying wide tables in psql

[Greg Stark]

On Tue, Apr 8, 2014 at 12:19 PM, Andres Freund and...@2ndquadrant.com wrote:
 I don't think this is easily testable that way - doesn't it rely on
 determining the width of the terminal? Which you won't have when started
 from pg_regress?

There's a pset variable to set the target width so at least the
formatting code can be tested. It would be nice to have the ioctl at
least get called on the regression farm so we're sure we aren't doing
something unportable.

-- 
greg


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

Re: [HACKERS] Problem with displaying wide tables in psql

[Emre Hasegeli]

2014-02-16 18:37, Sergey Muraviov sergey.k.murav...@gmail.com:

 New code doesn't work with empty strings but I've done minor optimization
 for this case.

It seems better now. I added some new lines and spaces, removed unnecessary
parentheses and marked it as Ready for Committer.


fix_psql_print_aligned_vertical_v5.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] Problem with displaying wide tables in psql

[Sergey Muraviov]

Thanks.


2014-02-17 12:22 GMT+04:00 Emre Hasegeli e...@hasegeli.com:

 2014-02-16 18:37, Sergey Muraviov sergey.k.murav...@gmail.com:

  New code doesn't work with empty strings but I've done minor optimization
  for this case.

 It seems better now. I added some new lines and spaces, removed unnecessary
 parentheses and marked it as Ready for Committer.




-- 
Best regards,
Sergey Muraviov