Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Tom Lane]

Christoph Berg  writes:
> Re: Tom Lane 2017-05-30 <1564.1496176...@sss.pgh.pa.us>
>> It'd be interesting if people could gather similar numbers on other
>> platforms of more real-world relevance, such as ppc64.  But based on
>> this small sample, I wouldn't object to just going to -fPIC across
>> the board.

> [ more numbers ]

OK, this looks good to me.  Just to make sure everyone's on the
same page, what I propose to do is simplify all our platform-specific
Makefiles that use either -fpic or -fPIC to use -fPIC unconditionally.
This affects the netbsd, linux, and openbsd ports.  Looks like we should
also change the example link commands in dfunc.sgml.

Next question: should we back-patch this change, or just do it in HEAD?

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] pg_class.relpartbound definition overly brittle

[Andres Freund]

On 2017-05-31 15:06:06 -0700, Mark Dilger wrote:
> That's cold comfort, given that most users will be looking at the pg_class
> table and not writing C code that compares Node objects.  I wrote a bit of
> regression test logic that checks, and sure enough the relpartbound field
> shows up as unequal:
>   
>   relpartbound
> 
> SELECT a.relpartbound, b.relpartbound, a.relpartbound = b.relpartbound, 
> a.relpartbound::text = b.relpartbound::text
> FROM pg_class a, pg_class b
> WHERE a.relname = 'acct_partitioned_1'
>   AND b.relname = 'acct_partitioned_2';
>   
>   relpartbound
>   
>   |   
>  relpartbound 
>   
>  | ?column? | ?column?
> ++--+--
>  {PARTITIONBOUNDSPEC :strategy l :listdatums ({CONST :consttype 23000 
> :consttypmod -1 :constcollid 0 :constlen 2 :constbyval true :constisnull 
> false :location -1 :constvalue 2 [ 0 0 0 0 0 0 0 0 ]}) :lowerdatums <> 
> :upperdatums <> :location 82} | {PARTITIONBOUNDSPEC :strategy l :listdatums 
> ({CONST :consttype 23000 :consttypmod -1 :constcollid 0 :constlen 2 
> :constbyval true :constisnull false :location -1 :constvalue 2 [ 0 0 0 0 0 0 
> 0 0 ]}) :lowerdatums <> :upperdatums <> :location 73} | f| f  
> (1 row)

Normal users aren't going to make sense of node trees in the first
place.  You should use pg_get_expr for it:
postgres[3008][1]=# SELECT pg_get_expr(relpartbound, oid) FROM pg_class WHERE 
relpartbound IS NOT NULL;
┌──┐
│ pg_get_expr  │
├──┤
│ FOR VALUES IN (1, 2) │
└──┘
(1 row)

- Andres


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

Re: [HACKERS] pg_class.relpartbound definition overly brittle

[Mark Dilger]

> On May 31, 2017, at 3:17 PM, Andres Freund  wrote:
> 
> On 2017-05-31 15:06:06 -0700, Mark Dilger wrote:
>> That's cold comfort, given that most users will be looking at the pg_class
>> table and not writing C code that compares Node objects.  I wrote a bit of
>> regression test logic that checks, and sure enough the relpartbound field
>> shows up as unequal:
>>  
>>   relpartbound   
>>  
>> SELECT a.relpartbound, b.relpartbound, a.relpartbound = b.relpartbound, 
>> a.relpartbound::text = b.relpartbound::text
>>FROM pg_class a, pg_class b
>>WHERE a.relname = 'acct_partitioned_1'
>>  AND b.relname = 'acct_partitioned_2';
>>  
>>   relpartbound   
>>   |  
>>   
>> relpartbound   | 
>> ?column? | ?column?
>> ++--+--
>> {PARTITIONBOUNDSPEC :strategy l :listdatums ({CONST :consttype 23000 
>> :consttypmod -1 :constcollid 0 :constlen 2 :constbyval true :constisnull 
>> false :location -1 :constvalue 2 [ 0 0 0 0 0 0 0 0 ]}) :lowerdatums <> 
>> :upperdatums <> :location 82} | {PARTITIONBOUNDSPEC :strategy l :listdatums 
>> ({CONST :consttype 23000 :consttypmod -1 :constcollid 0 :constlen 2 
>> :constbyval true :constisnull false :location -1 :constvalue 2 [ 0 0 0 0 0 0 
>> 0 0 ]}) :lowerdatums <> :upperdatums <> :location 73} | f| f  
>> (1 row)
> 
> Normal users aren't going to make sense of node trees in the first
> place.  You should use pg_get_expr for it:
> postgres[3008][1]=# SELECT pg_get_expr(relpartbound, oid) FROM pg_class WHERE 
> relpartbound IS NOT NULL;
> ┌──┐
> │ pg_get_expr  │
> ├──┤
> │ FOR VALUES IN (1, 2) │
> └──┘
> (1 row)

I concede that mitigates the problem somewhat, though I still think a user may 
look
at pg_class, see there is a column that appears to show the partition 
boundaries,
and then decide to check whether two tables have the same partition boundaries
by comparing those fields, without passing them first through pg_get_expr(), a
function they may never have heard of.

To me, it seems odd to immortalize a SQL parsing field in the catalog 
definition of
the relation, but perhaps that's just my peculiar sensibilities.  If the 
community is more
on your side, I'm not going to argue it.

Mark Dilger



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

Re: [HACKERS] pg_class.relpartbound definition overly brittle

[Andres Freund]

On 2017-05-31 15:38:58 -0700, Mark Dilger wrote:
> > Normal users aren't going to make sense of node trees in the first
> > place.  You should use pg_get_expr for it:
> > postgres[3008][1]=# SELECT pg_get_expr(relpartbound, oid) FROM pg_class 
> > WHERE relpartbound IS NOT NULL;
> > ┌──┐
> > │ pg_get_expr  │
> > ├──┤
> > │ FOR VALUES IN (1, 2) │
> > └──┘
> > (1 row)
> 
> I concede that mitigates the problem somewhat, though I still think a user 
> may look
> at pg_class, see there is a column that appears to show the partition 
> boundaries,
> and then decide to check whether two tables have the same partition boundaries
> by comparing those fields, without passing them first through pg_get_expr(), a
> function they may never have heard of.
> 
> To me, it seems odd to immortalize a SQL parsing field in the catalog 
> definition of
> the relation, but perhaps that's just my peculiar sensibilities.  If the 
> community is more
> on your side, I'm not going to argue it.

Given that various other node trees stored in the catalog also have
location fields, about which I recall no complaints, I don't think this
is a significant issue.  Nor something that I think makes sense to solve
in isolation, without tackling all stored expressions.

- Andres


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

Re: [HACKERS] logical replication - still unstable after all these months

[Erik Rijkers]

On 2017-05-31 11:16, Petr Jelinek wrote:
[...]

Thanks to Mark's offer I was able to study the issue as it happened and
found the cause of this.

[0001-Improve-handover-logic-between-sync-and-apply-worker.patch]


This looks good:

-- out_20170531_1141.txt
100 -- pgbench -c 90 -j 8 -T 60 -P 12 -n   --  scale 25
100 -- All is well.

So this is 100x a 1-minute test with 100x success. (This on the most 
fastidious machine (slow disks, meagre specs) that used to give 15% 
failures)


I'll let it run for a couple of days with varying params (and on varying 
hardware) but it definitely does look as if you fixed it.


Thanks!

Erik Rijkers


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

Re: [HACKERS] pg_class.relpartbound definition overly brittle

[Alvaro Herrera]

Mark Dilger wrote:
> Hackers,
> 
> recent changes have introduced the :location field to the partboundspec
> in pg_catalog.pg_class.  This means that if two identical tables with
> identical partitioning scheme are created, but one is done before a change
> to gram.y, and the other after a change to gram.y, the relpartbound fields
> for those two tables could show up as different.

Actually, if you look at equalfuncs.c, you'll note that locations are
not really compared:

/* Compare a parse location field (this is a no-op, per note above) */
#define COMPARE_LOCATION_FIELD(fldname) \
((void) 0)

where the referenced note is:

 * NOTE: it is intentional that parse location fields (in nodes that have
 * one) are not compared.  This is because we want, for example, a variable
 * "x" to be considered equal() to another reference to "x" in the query.

-- 
Álvaro Herrerahttps://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] pg_class.relpartbound definition overly brittle

[Mark Dilger]

> On May 31, 2017, at 2:49 PM, Alvaro Herrera  wrote:
> 
> Mark Dilger wrote:
>> Hackers,
>> 
>> recent changes have introduced the :location field to the partboundspec
>> in pg_catalog.pg_class.  This means that if two identical tables with
>> identical partitioning scheme are created, but one is done before a change
>> to gram.y, and the other after a change to gram.y, the relpartbound fields
>> for those two tables could show up as different.
> 
> Actually, if you look at equalfuncs.c, you'll note that locations are
> not really compared:
> 
> /* Compare a parse location field (this is a no-op, per note above) */
> #define COMPARE_LOCATION_FIELD(fldname) \
>   ((void) 0)
> 
> where the referenced note is:
> 
> * NOTE: it is intentional that parse location fields (in nodes that have
> * one) are not compared.  This is because we want, for example, a variable
> * "x" to be considered equal() to another reference to "x" in the query.

That's cold comfort, given that most users will be looking at the pg_class
table and not writing C code that compares Node objects.  I wrote a bit of
regression test logic that checks, and sure enough the relpartbound field
shows up as unequal:

relpartbound

SELECT a.relpartbound, b.relpartbound, a.relpartbound = b.relpartbound, 
a.relpartbound::text = b.relpartbound::text
FROM pg_class a, pg_class b
WHERE a.relname = 'acct_partitioned_1'
  AND b.relname = 'acct_partitioned_2';

relpartbound

|   
 relpartbound   

 | ?column? | ?column?
++--+--
 {PARTITIONBOUNDSPEC :strategy l :listdatums ({CONST :consttype 23000 
:consttypmod -1 :constcollid 0 :constlen 2 :constbyval true :constisnull false 
:location -1 :constvalue 2 [ 0 0 0 0 0 0 0 0 ]}) :lowerdatums <> :upperdatums 
<> :location 82} | {PARTITIONBOUNDSPEC :strategy l :listdatums ({CONST 
:consttype 23000 :consttypmod -1 :constcollid 0 :constlen 2 :constbyval true 
:constisnull false :location -1 :constvalue 2 [ 0 0 0 0 0 0 0 0 ]}) 
:lowerdatums <> :upperdatums <> :location 73} | f| f  
(1 row)





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

Re: [HACKERS] pg_class.relpartbound definition overly brittle

[Mark Dilger]

> On May 31, 2017, at 4:00 PM, Alvaro Herrera  wrote:
> 
> Mark Dilger wrote:
> 

relpartbound
  | 

 relpartbound   
 | ?column? | ?column?
 ++--+--
 {PARTITIONBOUNDSPEC :strategy l :listdatums ({CONST :consttype 23000 
 :consttypmod -1 :constcollid 0 :constlen 2 :constbyval true :constisnull 
 false :location -1 :constvalue 2 [ 0 0 0 0 0 0 0 0 ]}) :lowerdatums <> 
 :upperdatums <> :location 82} | {PARTITIONBOUNDSPEC :strategy l 
 :listdatums ({CONST :consttype 23000 :consttypmod -1 :constcollid 0 
 :constlen 2 :constbyval true :constisnull false :location -1 :constvalue 2 
 [ 0 0 0 0 0 0 0 0 ]}) :lowerdatums <> :upperdatums <> :location 73} | f
 | f  
 (1 row)
> 
>> I concede that mitigates the problem somewhat, though I still think a user 
>> may look
>> at pg_class, see there is a column that appears to show the partition 
>> boundaries,
>> and then decide to check whether two tables have the same partition 
>> boundaries
>> by comparing those fields, without passing them first through pg_get_expr(), 
>> a
>> function they may never have heard of.
> 
> How do you expect that the used would actually interpret the above
> weird-looking value?  There's no way to figure out what operations are
> being done in that ugly blob of text.

I don't know how the average user would use it.  I can only tell you what
I am doing, which may be on the fringe of what users do typically.

I have modified the system to add a number of catalog tables not normally
present in postgres.  A few of those catalog tables are partitioned.  Since
they have to be set up at bootstrap time, I can't use the CREATE TABLE
syntax in some src/backend/catalog/*.sql file to create them, I have to
create them with header files, genbki.pl and friends.  There is no support
for this, so I created my own.  That puts me at risk of getting out of sync
with the public sources implementation of partitioning.  As such, I have
regression tests that create at run time partitioned tables that have all the
same columns and boundaries as my catalog tables, and I compare the
pg_class entries against each other to make sure there are no inconsistencies.
When you guys commit changes that impact partitioning, I notice, and change
my code to match.  But in this case, it seemed to me the change that got
committed was not thought through, and it might benefit the community for
me to point it out, rather than quietly make my code behave the same as
what got committed.

I doubt too many people will follow in my footsteps here, but other people
may hit this :location peculiarity for other reasons.

Once again, this is just to give you context about why I said anything in the
first place.

> I suspect it would be better to reduce the location field to -1 as
> proposed, though, since the location has no actual meaning once the node
> is stored standalone rather than as part of a larger command.  However
> it's clear that we're not consistent about doing this elsewhere.

I have no opinion about whether this should be done for 10.0, given the
release schedule.  Changing it for 10.0 or 11.0 seems reasonable to me.

Mark Dilger

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

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Christoph Berg]

Re: Tom Lane 2017-05-31 <28752.1496238...@sss.pgh.pa.us>
> OK, this looks good to me.  Just to make sure everyone's on the
> same page, what I propose to do is simplify all our platform-specific
> Makefiles that use either -fpic or -fPIC to use -fPIC unconditionally.
> This affects the netbsd, linux, and openbsd ports.  Looks like we should
> also change the example link commands in dfunc.sgml.

Ack.

> Next question: should we back-patch this change, or just do it in HEAD?

Debian "needs" it for 9.6, but I've already pushed the s390x patch in
the original posting, so I could just live with it being just in head.
But of course it would be nice to have everything in sync.

Christoph


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

Re: [HACKERS] POC: Sharing record typmods between backends

[Robert Haas]

On Wed, May 31, 2017 at 11:16 AM, Dilip Kumar  wrote:
> I agree with you. But, if I understand the use case correctly we need
> to store the TupleDesc for the RECORD in shared hash so that it can be
> shared across multiple processes.  I think this can be achieved with
> the simplehash as well.
>
> For getting this done, we need some fixed shared memory for holding
> static members of SH_TYPE and the process which creates the simplehash
> will be responsible for copying these static members to the shared
> location so that other processes can access the SH_TYPE.  And, the
> dynamic part (the actual hash entries) can be allocated using DSA by
> registering SH_ALLOCATE function.

Well, SH_TYPE's members SH_ELEMENT_TYPE *data and void *private_data
are not going to work in DSM, because they are pointers.  You can
doubtless come up with a way around that problem, but I guess the
question is whether that's actually any better than just using DHT.

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


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

Re: [HACKERS] [PATCH] relocation truncated to fit: citus build failure on s390x

[Tom Lane]

Alvaro Herrera  writes:
> My vote would be to backpatch it all the way.

That's my thought too.  Otherwise it'll be five years before extension
authors can stop worrying about this issue.

regards, tom lane


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

Re: [HACKERS] TAP backpatching policy

[Tom Lane]

Alvaro Herrera  writes:
> My main concern is how widely is the buildfarm going to test the new
> test frameworks.  If we backpatch PostgresNode-based tests to 9.2, are
> buildfarm animals going to need to be reconfigured to use
> --enable-tap-tests?

Yes.  The animals that are doing it at all are using code more or less
like this:

if ($branch eq 'HEAD' or $branch ge 'REL9_4')
{
push(@{$conf{config_opts}},"--enable-tap-tests");
}

(verbatim from longfin's config script)

So maybe that's an argument for not going back before 9.4.  OTOH,
you may be giving the buildfarm owners too little credit for
willingness to update their configurations.

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

[Andres Freund]

On 2017-05-24 10:52:37 -0400, Robert Haas wrote:
> On Wed, May 24, 2017 at 10:32 AM, Andres Freund  wrote:
> > Well, but then we should just remove minval/maxval if we can't rely on
> > it.
>
> That seems like a drastic overreaction to me.

Well, either they work, or they don't.  But since it turns out to be
easy enough to fix anyway...


> > I wonder if that's not actually very little new code, and I think we
> > might end up regretting having yet another inconsistent set of semantics
> > in v10, which we'll then end up changing again in v11.
>
> I'm not exercised enough about it to spend time on it or to demand
> that Peter do so, but feel free to propose something.

This turns out to be fairly simple patch.  We already do precisely what
I describe when resetting a sequence for TRUNCATE, so it's an already
tested codepath.  It also follows a lot more established practice around
transactional schema changes, so I think that's architecturally better
too.  Peter, to my understanding, agreed with that reasoning at pgcon.

I just have two questions:
1) We've introduced, in 3d092fe540, infrastructure to avoid unnecessary
   catalog updates, in an attemt to fix the "concurrently updated"
   error. But that turned out to not be sufficient anyway, and it bulks
   up the code.  I'd vote for just removing that piece of logic, it
   doesn't buy us anything meaningful, and it's bulky.

2) There's currently logic that takes a lower level lock for ALTER
   SEQUENCE RESET without other options.  I think that's a bit confusing
   with the proposed change, because then it's unclear when ALTER
   SEQUENCE is transactional and when not.  I think it's actually a lot
   easier to understand if we keep nextval()/setval() as
   non-transactional, and ALTER SEQUENCE as transactional.

Comments?

- Andres


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

Re: [HACKERS] UPDATE of partition key

[Robert Haas]

On Mon, May 29, 2017 at 5:26 AM, Amit Kapila  wrote:
>> But I think, we can also take step-by-step approach even for v11. If
>> we agree that it is ok to silently do the updates as long as we
>> document the behaviour, we can go ahead and do this, and then as a
>> second step, implement error handling as a separate patch. If that
>> patch does not materialize, we at least have the current behaviour
>> documented.
>
> I think that is sensible approach if we find the second step involves
> big or complicated changes.

I think it is definitely a good idea to separate the two patches.
UPDATE tuple routing without any special handling for the EPQ issue is
just a partitioning feature.  The proposed handling for the EPQ issue
is an *on-disk format change*.  That turns a patch which is subject
only to routine bugs into one which can eat your data permanently --
so having the "can eat your data permanently" separated out for both
review and commit seems only prudent.  For me, it's not a matter of
which patch is big or complicated, but rather a matter of one of them
being a whole lot riskier than the other.  Even UPDATE tuple routing
could mess things up pretty seriously if we end up with tuples in the
wrong partition, of course, but the other thing is still worse.

In terms of a development plan, I think we would need to have both
patches before either could be committed.  I believe that everyone
other than me who has expressed an opinion on this issue has said that
it's unacceptable to just ignore the issue, so it doesn't sound like
there will be much appetite for having #1 go into the tree without #2.
I'm still really concerned about that approach because we do not have
very much bit space left and WARM wants to use quite a bit of it.  I
think it's quite possible that we'll be sad in the future if we find
that we can't implement feature XYZ because of the bit-space consumed
by this feature.  However, I don't have the only vote here and I'm not
going to try to shove this into the tree over multiple objections
(unless there are a lot more votes the other way, but so far there's
no sign of that).

Greg/Amit's idea of using the CTID field rather than an infomask bit
seems like a possibly promising approach.  Not everything that needs
bit-space can use the CTID field, so using it is a little less likely
to conflict with something else we want to do in the future than using
a precious infomask bit.  However, I'm worried about this:

/* Make sure there is no forward chain link in t_ctid */
tp.t_data->t_ctid = tp.t_self;

The comment does not say *why* we need to make sure that there is no
forward chain link, but it implies that some code somewhere in the
system does or at one time did depend on no forward link existing.
Any such code that still exists will need to be updated.  Anybody know
what code that might be, exactly?

The other potential issue I see here is that I know the WARM code also
tries to use the bit-space in the CTID field; in particular, it uses
the CTID field of the last tuple in a HOT chain to point back to the
root of the chain.  That seems like it could conflict with the usage
proposed here, but I'm not totally sure.  Has anyone investigated this
issue?

Regarding the trigger issue, I can't claim to have a terribly strong
opinion on this.  I think that practically anything we do here might
upset somebody, but probably any halfway-reasonable thing we choose to
do will be OK for most people.  However, there seems to be a
discrepancy between the approach that got the most votes and the one
that is implemented by the v8 patch, so that seems like something to
fix.

For what it's worth, in the future, I imagine that we might allow
adding a trigger to a partitioned table and having that cascade down
to all descendant tables.  In that world, firing the BR UPDATE trigger
for the old partition and the AR UPDATE trigger for the new partition
will look a lot like the behavior the user would expect on an
unpartitioned table, which could be viewed as a good thing.  On the
other hand, it's still going to be a DELETE+INSERT under the hood for
the foreseeable future, so firing the delete triggers and then the
insert triggers is also defensible.  Is there any big difference
between these appraoches in terms of how much code is required to make
this work?

In terms of the approach taken by the patch itself, it seems
surprising to me that the patch only calls
ExecSetupPartitionTupleRouting when an update fails the partition
constraint.  Note that in the insert case, we call that function at
the start of execution; calling it in the middle seems to involve
additional hazards; for example, is it really safe to add additional
ResultRelInfos midway through the operation?  Is it safe to take more
locks midway through the operation? It seems like it might be a lot
safer to decide at the beginning of the operation whether this is
needed -- we can skip it if none of the columns involved in the

[HACKERS] COPY (query) TO ... doesn't allow parallelism

[Andres Freund]

Hi,

At the moment $subject doesn't allow parallelism, because copy.c's
pg_plan_query() invocation doesn't set the CURSOR_OPT_PARALLEL_OK
flag.

To me that appears to be an oversight rather than intentional.  A
somewhat annoying one at that, because it's not uncommong to use COPY to
execute reports to a CSV file and such.

Robert, am I missing a complication?

I personally think we should fix this in 9.6 and 10, but I've to admit
I'm not entirely impartial, because Citus hit this...

Greetings,

Andres Freund


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

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

[Robert Haas]

On Fri, May 26, 2017 at 10:51 AM, Magnus Hagander  wrote:
> I would definitely suggest putting it in HEAD (and thus, v10) for a while to
> get some real world exposure before backpatching. But if it does work out
> well in the end, then we can certainly consider backpatching it. But given
> the difficulty in reliably reproducing the problem etc, I think it's a good
> idea to give it some proper real world experience in 10 first.

So, are you going to, perhaps, commit this?  Or who is picking this up?

/me knows precious little about Windows.

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


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

Re: [HACKERS] pg_class.relpartbound definition overly brittle

[Mark Dilger]

> On May 31, 2017, at 3:42 PM, Andres Freund  wrote:
> 
> On 2017-05-31 15:38:58 -0700, Mark Dilger wrote:
>>> Normal users aren't going to make sense of node trees in the first
>>> place.  You should use pg_get_expr for it:
>>> postgres[3008][1]=# SELECT pg_get_expr(relpartbound, oid) FROM pg_class 
>>> WHERE relpartbound IS NOT NULL;
>>> ┌──┐
>>> │ pg_get_expr  │
>>> ├──┤
>>> │ FOR VALUES IN (1, 2) │
>>> └──┘
>>> (1 row)
>> 
>> I concede that mitigates the problem somewhat, though I still think a user 
>> may look
>> at pg_class, see there is a column that appears to show the partition 
>> boundaries,
>> and then decide to check whether two tables have the same partition 
>> boundaries
>> by comparing those fields, without passing them first through pg_get_expr(), 
>> a
>> function they may never have heard of.
>> 
>> To me, it seems odd to immortalize a SQL parsing field in the catalog 
>> definition of
>> the relation, but perhaps that's just my peculiar sensibilities.  If the 
>> community is more
>> on your side, I'm not going to argue it.
> 
> Given that various other node trees stored in the catalog also have
> location fields, about which I recall no complaints, I don't think this
> is a significant issue.  Nor something that I think makes sense to solve
> in isolation, without tackling all stored expressions.

Ok.  Just to clarify, I didn't come up with this problem as part of some general
code review.  I merged the recent changes into my tree, and my regression
tests broke.  That's fine; that's what they are there to do.  Break, and in so 
doing,
alert me to the fact that the project has changed things that will require me to
make modifications.  It just seemed strange to me that I should be changing 
stuff
to try to get the :location field to be equal in my code to the :location field 
in the
public sources, since it seems to serve no purpose.

Mark Dilger

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

Re: [HACKERS] pg_class.relpartbound definition overly brittle

[Alvaro Herrera]

Mark Dilger wrote:

> >>
> >> relpartbound   
> >>   |
> >>
> >>  relpartbound  
> >>  | ?column? | ?column?
> >> ++--+--
> >> {PARTITIONBOUNDSPEC :strategy l :listdatums ({CONST :consttype 23000 
> >> :consttypmod -1 :constcollid 0 :constlen 2 :constbyval true :constisnull 
> >> false :location -1 :constvalue 2 [ 0 0 0 0 0 0 0 0 ]}) :lowerdatums <> 
> >> :upperdatums <> :location 82} | {PARTITIONBOUNDSPEC :strategy l 
> >> :listdatums ({CONST :consttype 23000 :consttypmod -1 :constcollid 0 
> >> :constlen 2 :constbyval true :constisnull false :location -1 :constvalue 2 
> >> [ 0 0 0 0 0 0 0 0 ]}) :lowerdatums <> :upperdatums <> :location 73} | f
> >> | f  
> >> (1 row)

> I concede that mitigates the problem somewhat, though I still think a user 
> may look
> at pg_class, see there is a column that appears to show the partition 
> boundaries,
> and then decide to check whether two tables have the same partition boundaries
> by comparing those fields, without passing them first through pg_get_expr(), a
> function they may never have heard of.

How do you expect that the used would actually interpret the above
weird-looking value?  There's no way to figure out what operations are
being done in that ugly blob of text.

I suspect it would be better to reduce the location field to -1 as
proposed, though, since the location has no actual meaning once the node
is stored standalone rather than as part of a larger command.  However
it's clear that we're not consistent about doing this elsewhere.

> To me, it seems odd to immortalize a SQL parsing field in the catalog 
> definition of
> the relation, but perhaps that's just my peculiar sensibilities.  If the 
> community is more
> on your side, I'm not going to argue it.


-- 
Álvaro Herrerahttps://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