Thread: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitrary vacuum flags

[HACKERS] Proposal: GetOldestXminExtend for ignoring arbitrary vacuum flags

From
"Seki, Eiji"
Date:
Hi all, 

I propose the patch that adds a function GetOldestXminExtended that is like GetOldestXmin but can ignore arbitrary
vacuumflags. And then, rewrite GetOldestXmin to use it. Note that this is done so as not to change the behavior of
GetOldestXmin.

This change will be useful for features that only reads rows that are visible by all transactions and could not wait
specificprocesses (VACUUM, ANALYZE, etc...) for performance. Our company (Fujitsu) is developing such an extension. In
ourbenchmark, we found that waiting an ANALYZE process created by autovacuum daemon often has a significant impact to
theperformance although the waited process do only reading as to the table. So I hope to ignore it using
GetOldestXminExtendas below. The second argument represents flags to ignore.
 

  GetOldestXminExtended(rel, PROC_IN_VACUUM | PROC_IN_LOGICAL_DECODING | PROC_IN_ANALYZE);

Note: We can ignore VACUUM processes or VACUUM with ANALYZE processes using the second option of GetOldesXmin,
"ignoreVacuum".However, we cannot ignore only ANALYZE processes because the "ignoreVacuum" = true is same to the
following.

  GetOldestXminExtended(rel, PROC_IN_VACUUM | PROC_IN_LOGICAL_DECODING)

This change should have no impact to the existing GetOldestXmin without slight overhead to call the function.

I'm not sure that this feature is useful in general.
Please let me know your opinion if you are interested.

Regards,
Eiji Seki
Fujitsu.

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

Attachment

Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Michael Paquier
Date:
On Tue, Feb 14, 2017 at 3:19 PM, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:
> This change will be useful for features that only reads rows that are visible by all transactions and could not wait
specificprocesses (VACUUM, ANALYZE, etc...) for performance. Our company (Fujitsu) is developing such an extension. In
ourbenchmark, we found that waiting an ANALYZE process created by autovacuum daemon often has a significant impact to
theperformance although the waited process do only reading as to the table. So I hope to ignore it using
GetOldestXminExtendas below. The second argument represents flags to ignore. 
>
>   GetOldestXminExtended(rel, PROC_IN_VACUUM | PROC_IN_LOGICAL_DECODING | PROC_IN_ANALYZE);
>
> Note: We can ignore VACUUM processes or VACUUM with ANALYZE processes using the second option of GetOldesXmin,
"ignoreVacuum".However, we cannot ignore only ANALYZE processes because the "ignoreVacuum" = true is same to the
following.
GetOldestXmin(Relation rel, bool ignoreVacuum){
+   uint8 ignoreFlags = PROC_IN_LOGICAL_DECODING;
+
+   if (ignoreVacuum)
+       ignoreFlags |= PROC_IN_VACUUM;
+
+   return GetOldestXminExtended(rel, ignoreFlags);
+}

It seems to me that it would be far less confusing to just replace the
boolean argument of GetOldestXmin by a uint8 and get those flags
declared in procarray.h, no? There are a couple of code path calling
GetOldestXmin() that do not include proc.h, so it would be better to
not add any extra header dependencies in those files.
--
Michael



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
"Seki, Eiji"
Date:
Michael Paquier <michael.paquier@gmail.com> wrote:
> It seems to me that it would be far less confusing to just replace the boolean argument of GetOldestXmin by a uint8
andget those flags declared in procarray.h, no? There are a couple of code path calling
 
> GetOldestXmin() that do not include proc.h, so it would be better to not add any extra header dependencies in those
files.

Thank you for your feedback.

Yes, I agree that such extra dependencies are not desirable. I understood that such as the following implementation is
better(I attach a patch for reference). Please let me know if my understanding is not correct.
 

1. Change the arguments of GetOldestXmin
  -extern TransactionId GetOldestXmin(Relation rel, bool ignoreVacuum);
  +extern TransactionId GetOldestXmin(Relation rel, uint8 ignoreFlags);

2. Add IGNORE_FLAGS_XXX for above "ignoreFlags" in procarray.h
  These flags are converted to combined PROC_XXX flag in procarray.c before ignoring

3. Fix callers of GetOldestXmin
  GetOldestXmin(NULL, true)  -> GetOldestXmin(NULL, IGNORE_FLAGS_VACUUM)
  GetOldestXmin(NULL, false) -> GetOldestXmin(NULL, IGNORE_FLAGS_DEFAULT)

Regards,
Eiji Seki
Fujitsu.

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Michael Paquier
Sent: Tuesday, February 14, 2017 3:43 PM
To: Seki, Eiji/関 栄二
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitrary vacuum flags

On Tue, Feb 14, 2017 at 3:19 PM, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:
> This change will be useful for features that only reads rows that are visible by all transactions and could not wait
specificprocesses (VACUUM, ANALYZE, etc...) for performance. Our company (Fujitsu) is developing such an extension. In
ourbenchmark, we found that waiting an ANALYZE process created by autovacuum daemon often has a significant impact to
theperformance although the waited process do only reading as to the table. So I hope to ignore it using
GetOldestXminExtendas below. The second argument represents flags to ignore.
 
>
>   GetOldestXminExtended(rel, PROC_IN_VACUUM | PROC_IN_LOGICAL_DECODING 
> | PROC_IN_ANALYZE);
>
> Note: We can ignore VACUUM processes or VACUUM with ANALYZE processes using the second option of GetOldesXmin,
"ignoreVacuum".However, we cannot ignore only ANALYZE processes because the "ignoreVacuum" = true is same to the
following.

 GetOldestXmin(Relation rel, bool ignoreVacuum)  {
+   uint8 ignoreFlags = PROC_IN_LOGICAL_DECODING;
+
+   if (ignoreVacuum)
+       ignoreFlags |= PROC_IN_VACUUM;
+
+   return GetOldestXminExtended(rel, ignoreFlags); }

It seems to me that it would be far less confusing to just replace the boolean argument of GetOldestXmin by a uint8 and
getthose flags declared in procarray.h, no? There are a couple of code path calling
 
GetOldestXmin() that do not include proc.h, so it would be better to not add any extra header dependencies in those
files.
--
Michael


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

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

Attachment

Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Amit Kapila
Date:
On Tue, Feb 14, 2017 at 11:49 AM, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:
> Hi all,
>
> I propose the patch that adds a function GetOldestXminExtended that is like GetOldestXmin but can ignore arbitrary
vacuumflags. And then, rewrite GetOldestXmin to use it. Note that this is done so as not to change the behavior of
GetOldestXmin.
>
> This change will be useful for features that only reads rows that are visible by all transactions and could not wait
specificprocesses (VACUUM, ANALYZE, etc...) for performance.
 
>

How will you decide just based on oldest xmin whether the tuple is
visible or not?  How will you take decisions about tuples which have
xmax set?

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



On 2/14/17 3:13 AM, Seki, Eiji wrote:
>   +extern TransactionId GetOldestXmin(Relation rel, uint8 ignoreFlags);

My impression is that most other places that do this sort of thing just 
call the argument 'flags', so as not to "lock in" a single idea of what 
the flags are for. I can't readily think of another use for flags in 
GetOldestXmin, but ISTM it's better to just go with "flags" instead of 
"ignoreFlags".
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
"Seki, Eiji"
Date:
Amit Kapila wrote:
> How will you decide just based on oldest xmin whether the tuple is visible or not?  How will you take decisions about
tupleswhich have xmax set?
 

In our use case, GetOldestXmin is used by an original maintainer process[es] to an original control table[s]. The table
canbe concurrently read or inserted in any transactions. However, rows in the table can be deleted (set xmax) only by
themaintainer process. Then, one control table can be processed by only one maintainer process at once.
 

So I do MVCC as following.

- The maintainer's transaction:  - If xmax is set, simply ignore the tuple. - For other tuples, read tuples if
GetOldestXmin()> xmin.
 
- Other transactions: Do ordinal MVCC using his XID.

Note: A control table relates to a normal table relation, so get oldest xmin as to the normal table.
--
Regards,
Eiji Seki
Fujitsu



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
"Seki, Eiji"
Date:
Jim Nasby wrote:
> On 2/14/17 3:13 AM, Seki, Eiji wrote:
> >   +extern TransactionId GetOldestXmin(Relation rel, uint8 
> > ignoreFlags);
> 
> My impression is that most other places that do this sort of thing just call the argument 'flags', so as not to "lock
in"a single idea of what the flags are for. I can't readily think of another use for flags in GetOldestXmin, but ISTM
it'sbetter to just go with "flags" instead of "ignoreFlags".
 

Thanks. I also think "flags" is better. I will rename it.

But I wonder if I should rename the defined flag names, IGNORE_A_FLAG_XXX and IGNORE_FLAGS_XXX because they include
"IGNORE"in their name. I'm concerned GetOldestXmin users are difficult to know the meaning if they have general names,
andgeneral names will conflict to other definitions. Would you let me know if you have any idea?
 

--
Regards,
Eiji Seki
Fujitsu



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Simon Riggs
Date:
On 14 February 2017 at 06:19, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:
> In our benchmark, we found that waiting an ANALYZE process created by autovacuum daemon often has a significant
impactto the performance although the waited process do only reading as to the table.
 
...
> I'm not sure that this feature is useful in general.
> Please let me know your opinion if you are interested.

You mention the above problem and hypothesise a solution.

IMHO the discussion on this is the wrong way around. First we must be
certain that the solution is effective and has no problems, then we
decide how to code it or discuss APIs.

If you do this, ANALYZE will see an inconsistent view of data in the
table, biasing its view towards data not recently updated, which might
also have a negative performance effect.

Please persuade us with measurements that allowing this impact on
ANALYZE would really improve performance at least in your case, and
also examine the effect of this on the accuracy and usefulness of the
gathered statistics.

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



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Michael Paquier
Date:
On Wed, Feb 15, 2017 at 5:32 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> Please persuade us with measurements that allowing this impact on
> ANALYZE would really improve performance at least in your case, and
> also examine the effect of this on the accuracy and usefulness of the
> gathered statistics.

FWIW, there was last week a developer meeting in Tokyo, and Seki-san
has presented such measurements. So it is not like nothing exists in
this area.
-- 
Michael



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Alvaro Herrera
Date:
Michael Paquier wrote:
> On Wed, Feb 15, 2017 at 5:32 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> > Please persuade us with measurements that allowing this impact on
> > ANALYZE would really improve performance at least in your case, and
> > also examine the effect of this on the accuracy and usefulness of the
> > gathered statistics.
> 
> FWIW, there was last week a developer meeting in Tokyo, and Seki-san
> has presented such measurements. So it is not like nothing exists in
> this area.

Sounds like it'd be a good idea to present these results to this list,
too.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Amit Kapila
Date:
On Wed, Feb 15, 2017 at 12:03 PM, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:
> Amit Kapila wrote:
>> How will you decide just based on oldest xmin whether the tuple is visible or not?  How will you take decisions
abouttuples which have xmax set? 
>
> In our use case, GetOldestXmin is used by an original maintainer process[es] to an original control table[s]. The
tablecan be concurrently read or inserted in any transactions. However, rows in the table can be deleted (set xmax)
onlyby the maintainer process. Then, one control table can be processed by only one maintainer process at once. 
>
> So I do MVCC as following.
>
> - The maintainer's transaction:
>   - If xmax is set, simply ignore the tuple.
>   - For other tuples, read tuples if GetOldestXmin() > xmin.
> - Other transactions: Do ordinal MVCC using his XID.
>

Oh, this is a very specific case for which such an API can be useful.
Earlier, I have seen that people proposing some kind of hooks which
can be used for their specific purpose but exposing an API or changing
the signature of an API sound bit awkward.  Consider tomorrow someone
decides to change this API for some reason, it might become difficult
to decide because we can't find it's usage.

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



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Robert Haas
Date:
On Tue, Feb 14, 2017 at 1:38 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
> On 2/14/17 3:13 AM, Seki, Eiji wrote:
>>   +extern TransactionId GetOldestXmin(Relation rel, uint8 ignoreFlags);
>
>
> My impression is that most other places that do this sort of thing just call
> the argument 'flags', so as not to "lock in" a single idea of what the flags
> are for. I can't readily think of another use for flags in GetOldestXmin,
> but ISTM it's better to just go with "flags" instead of "ignoreFlags".

I agree; also, many years ago a guy named Tom Lane told me that flags
argument should typically be declared as type "int".  I've followed
that advice ever since.

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



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Andres Freund
Date:
On 2017-02-15 12:27:11 -0500, Robert Haas wrote:
> On Tue, Feb 14, 2017 at 1:38 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
> > On 2/14/17 3:13 AM, Seki, Eiji wrote:
> >>   +extern TransactionId GetOldestXmin(Relation rel, uint8 ignoreFlags);
> >
> >
> > My impression is that most other places that do this sort of thing just call
> > the argument 'flags', so as not to "lock in" a single idea of what the flags
> > are for. I can't readily think of another use for flags in GetOldestXmin,
> > but ISTM it's better to just go with "flags" instead of "ignoreFlags".
> 
> I agree; also, many years ago a guy named Tom Lane told me that flags
> argument should typically be declared as type "int".  I've followed
> that advice ever since.

Why is that?  I think uint makes a lot more sense for flags where the
flags are individual bits that set/unset. Doing that with the sign bit
isn't a good idea.

Andres



Andres Freund <andres@anarazel.de> writes:
> On 2017-02-15 12:27:11 -0500, Robert Haas wrote:
>> I agree; also, many years ago a guy named Tom Lane told me that flags
>> argument should typically be declared as type "int".  I've followed
>> that advice ever since.

> Why is that?  I think uint makes a lot more sense for flags where the
> flags are individual bits that set/unset. Doing that with the sign bit
> isn't a good idea.

It would only matter if you got to the point of needing the sign bit
for a flag, which basically never happens for this sort of usage
(or if it does, you'd better be thinking about switching to int64
anyway).  So the extra mental and typing load of choosing some other
type than plain old "int" isn't really justified, IMO.

Anyway the real point is that "int" is preferable to "uint8" which
was the original choice here.  "uint8" unduly constrains the number
of flags you can add without an ABI break, and on many machines it's
more efficient to pass "int" function arguments than some other width.
        regards, tom lane



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Haribabu Kommi
Date:


On Wed, Feb 15, 2017 at 11:35 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Feb 15, 2017 at 12:03 PM, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:
> Amit Kapila wrote:
>> How will you decide just based on oldest xmin whether the tuple is visible or not?  How will you take decisions about tuples which have xmax set?
>
> In our use case, GetOldestXmin is used by an original maintainer process[es] to an original control table[s]. The table can be concurrently read or inserted in any transactions. However, rows in the table can be deleted (set xmax) only by the maintainer process. Then, one control table can be processed by only one maintainer process at once.
>
> So I do MVCC as following.
>
> - The maintainer's transaction:
>   - If xmax is set, simply ignore the tuple.
>   - For other tuples, read tuples if GetOldestXmin() > xmin.
> - Other transactions: Do ordinal MVCC using his XID.
>

Oh, this is a very specific case for which such an API can be useful.
Earlier, I have seen that people proposing some kind of hooks which
can be used for their specific purpose but exposing an API or changing
the signature of an API sound bit awkward.  Consider tomorrow someone
decides to change this API for some reason, it might become difficult
to decide because we can't find it's usage.

The proposed change of new API is in the context of fixing the performance
problem of vertical clustered index feature [1].

During the performance test of VCI in parallel with OLTP queries, sometimes
the query performance is getting dropped because of not choosing the VCI
as the best plan. This is due to increase of more records in WOS relation
that are needed to be moved to ROS. If there are more records in WOS
relation, the it takes more time to generate the LOCAL ROS, because of
this reason, the VCI plan is not chosen.

Why there are more records in WOS? We used GetOldestXmin() function
to identify the minimum transaction that is present in the cluster in order to
move the data from WOS to ROS. This function doesn't ignore the ANALYZE
transactions that are running in the system. As these transactions doesn't
do any changes that will affect the data movement from WOS to ROS.

Because of the above reason, we need a new API or some change in API
to provide the Oldest xmin by ignoring the ANALYZE transactions, so that
it will reduce the size of WOS and improves the VCI query performance.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Michael Paquier
Date:
On Thu, Feb 16, 2017 at 10:48 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> Because of the above reason, we need a new API or some change in API
> to provide the Oldest xmin by ignoring the ANALYZE transactions, so that
> it will reduce the size of WOS and improves the VCI query performance.

A new API in core is not completely mandatory either. As mentioned
during last week developer meeting to Seki-san, as the list of PGXACT
and PGPROC is in shared memory you could as well develop your own
version. Still there is a limitation with
KnownAssignedXidsGetOldestXmin in recovery, which may be better if
exposed at quick glance but you actually don't care for storage,
right? If the change makes sense, it seems to me that making a routine
more extensible makes the most sense in this case if the API extension
can be used by modules with more goals in mind.
-- 
Michael



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitrary vacuum flags

From
"Seki, Eiji"
Date:
Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Please persuade us with measurements that allowing this impact on
> ANALYZE would really improve performance at least in your case, and
> also examine the effect of this on the accuracy and usefulness of the
> gathered statistics.

I explain results of the test that Haribabu mentioned in [1].

The purpose of this test is the followings.

- Check the effect of VCI to OLTP workload
- Check whether VCI can be used in OLAP query 
  even if there is OLTP workload at a same table

The test is done as the followings.

- Use the Tiny TPC-C [2] benchmark as OLTP workload
    - Scale factor: 100
- Create VCI on the 'stock' table before starting benchmark
    - Planner doesn't select VCI for queries of the Tiny TPC-C.

I attach the result graph.

This graph indicates the transition of the number of rows in WOS. In our environment, when the WOS size exceeds about
700,000,VCI is no longer used as such the following query.
 

select count(*) from stock where s_order_cnt > 4;

While in low load ("Number of clients = 10" line, the throughput was about 1,000) the WOS size didn't exceed about
500,000,in high load ("Number of clients = 30 (Without patch)" line, the throughput was about 1,400) the WOS size
frequentlyexceeded 700,000.
 

While the WOS size continued to increase, ANALYZE only (without VACUUM) process created by autovacuum daemon always ran
andconversion process from WOS to ROS didn't run. Then, after changing to ignore ANALYZE only processes using my patch,
theWOS size no longer exceeded about 500,000 ("Number of clients = 30 (With patch)" line, the throughput was about
1,400).

Please let me know if you need any further information.

[1] - https://www.postgresql.org/message-id/CAJrrPGen1bJYRHu7VFp13QZUyaLdX5N4AH1cqQdiNd8uLVZWow%40mail.gmail.com
[2] - http://hp.vector.co.jp/authors/VA052413/jdbcrunner/manual_ja/tpc-c.html (Sorry, there is Japanese document only)

--
Regards,
Eiji Seki
Fujitsu




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

Attachment

Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
"Seki, Eiji"
Date:
On 2017-02-15 17:27:11     Robert Haas wrote:
> On Tue, Feb 14, 2017 at 1:38 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> > On 2/14/17 3:13 AM, Seki, Eiji wrote:
> >>   +extern TransactionId GetOldestXmin(Relation rel, uint8 ignoreFlags);
> >
> >
> > My impression is that most other places that do this sort of thing just call
> > the argument 'flags', so as not to "lock in" a single idea of what the flags
> > are for. I can't readily think of another use for flags in GetOldestXmin,
> > but ISTM it's better to just go with "flags" instead of "ignoreFlags".
> 
> I agree; also, many years ago a guy named Tom Lane told me that flags
> argument should typically be declared as type "int".  I've followed
> that advice ever since.

Thank you for your comments.

I reflected these comments to the attached patch. And I renamed IGNORE_XXX flags to PROCARRAY_XXX flags.

--
Regards,
Eiji Seki
Fujitsu



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

Attachment

Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Simon Riggs
Date:
On 16 February 2017 at 05:24, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:
> Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Please persuade us with measurements that allowing this impact on
>> ANALYZE would really improve performance at least in your case, and
>> also examine the effect of this on the accuracy and usefulness of the
>> gathered statistics.
>
> I explain results of the test that Haribabu mentioned in [1].

Thanks for the tests. I can see the performance is affected by this.


> Please let me know if you need any further information.

...you didn't comment at all on the accuracy and usefulness of the
gathered statistics, when the sample is biased towards non-updated
data.

I'm wondering whether this should be an additional table-level option.

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



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Craig Ringer
Date:
On 14 February 2017 at 14:19, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:
> Hi all,
>
> I propose the patch that adds a function GetOldestXminExtended that is like GetOldestXmin but can ignore arbitrary
vacuumflags. And then, rewrite GetOldestXmin to use it. Note that this is done so as not to change the behavior of
GetOldestXmin.

FWIW, I have changes in the logical decoding on standby patch that
also need to extend GetOldestXmin. Specifically, I need to be able to
return the catalog_xmin separately, rather than the current behaviour
of returning what's effectively min(xmin,catalog_xmin).

It's only loosely related to this change, but may be relevant.

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



Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
"Seki, Eiji"
Date:
on 2017-02-24 04:41:09 Simon Riggs wrote:
> ...you didn't comment at all on the accuracy and usefulness of the
> gathered statistics, when the sample is biased towards non-updated
> data.

In my understanding, the sample for statistics is not biased at least in our use case because the conversion process
fromWOS to ROS never modify (but only read) indexed table and modify only an index relation. And I think such a process
thatmodifies some tables should not ignore analyze processes.
 

--
Regards,
Eiji Seki
Fujitsu




Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Haribabu Kommi
Date:
On Fri, Feb 24, 2017 at 3:17 PM, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:

Thank you for your comments.

I reflected these comments to the attached patch. And I renamed IGNORE_XXX flags to PROCARRAY_XXX flags.

I checked the latest patch and I have some comments.

+static int
+ConvertProcarrayFlagToProcFlag(int flags)

I feel this function is not needed, if we try to maintain same flag values
for both PROC_XXX and PROCARRAY_XXX by writing some comments
in the both the declarations place to make sure that the person modifying
the flag values needs to update them in both the places. I feel it is usually
rare that the flag values gets changed.

+ * Now, flags is used only for ignoring backends with some flags. 

How about writing something like below.

The flags are used to ignore the backends in calculation when any of the
corresponding flags is set.

+#define PROCARRAY_A_FLAG_VACUUM

How about changing the flag name to PROCARRAY_VACUUM_FLAG
(similar changes to other declarations)


+/* Use these flags in GetOldestXmin as "flags" */

Add some comments here to explain how to use these flags.

+#define PROCARRAY_FLAGS_DEFAULT

same here also as PROCARRAY_DEFAULT_FLAGS
(similar changes to other declarations)


Regards,
Hari Babu
Fujitsu Australia
On Thu, Mar 16, 2017 at 12:29 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Fri, Feb 24, 2017 at 3:17 PM, Seki, Eiji <seki.eiji@jp.fujitsu.com>
> wrote:
>> Thank you for your comments.
>>
>> I reflected these comments to the attached patch. And I renamed IGNORE_XXX
>> flags to PROCARRAY_XXX flags.
>
> I checked the latest patch and I have some comments.
>
> +static int
> +ConvertProcarrayFlagToProcFlag(int flags)
>
> I feel this function is not needed, if we try to maintain same flag values
> for both PROC_XXX and PROCARRAY_XXX by writing some comments
> in the both the declarations place to make sure that the person modifying
> the flag values needs to update them in both the places. I feel it is
> usually
> rare that the flag values gets changed.

Yeah, it doesn't seem like a good idea to add additional computation
to something that's already a known hot spot.

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



On 2017-02-24 04:17:20 Haribabu Kommi wrote:
>On Fri, Feb 24, 2017 at 3:17 PM, Seki, Eiji <seki(dot)eiji(at)jp(dot)fujitsu(dot)com>
>wrote:
>
>>
>> Thank you for your comments.
>>
>> I reflected these comments to the attached patch. And I renamed IGNORE_XXX
>> flags to PROCARRAY_XXX flags.
>
>
>I checked the latest patch and I have some comments.
>
>+static int
>+ConvertProcarrayFlagToProcFlag(int flags)
>
>I feel this function is not needed, if we try to maintain same flag values
>for both PROC_XXX and PROCARRAY_XXX by writing some comments
>in the both the declarations place to make sure that the person modifying
>the flag values needs to update them in both the places. I feel it is
>usually
>rare that the flag values gets changed.
>
>+ * Now, flags is used only for ignoring backends with some flags.
>
>How about writing something like below.
>
>The flags are used to ignore the backends in calculation when any of the
>corresponding flags is set.
>
>+#define PROCARRAY_A_FLAG_VACUUM
>
>How about changing the flag name to PROCARRAY_VACUUM_FLAG
>(similar changes to other declarations)
>
>
>+/* Use these flags in GetOldestXmin as "flags" */
>
>Add some comments here to explain how to use these flags.
>
>+#define PROCARRAY_FLAGS_DEFAULT
>
>same here also as PROCARRAY_DEFAULT_FLAGS
>(similar changes to other declarations)

Thank you for you review.

I reflected your comment and attach the updated patch.

--
Regards,
Eiji Seki
Fujitsu

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

Attachment

Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Haribabu Kommi
Date:


On Tue, Mar 21, 2017 at 3:16 PM, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:

Thank you for you review.

I reflected your comment and attach the updated patch.

Thanks for the updated patch.

+/* Use these flags in GetOldestXmin as "flags" */

How about some thing like the following.
/* Use the following flags as an input "flags" to GetOldestXmin function */


+/* Ignore vacuum backends (Note: this also ignores analyze with vacuum backends) */
+#define PROCARRAY_FLAGS_VACUUM PROCARRAY_FLAGS_DEFAULT | PROCARRAY_VACUUM_FLAG
+/* Ignore analyze backends (Note: this also ignores vacuum with analyze backends) */
+#define PROCARRAY_FLAGS_ANALYZE PROCARRAY_FLAGS_DEFAULT | PROCARRAY_ANALYZE_FLAG

Whenever the above flags are passed to the GetOldestXmin() function,
it just verifies whether any one of the flags are set in the backend flags
or not. And also actually the PROC_IN_ANALYZE flag will set when
analyze operation is started and reset at the end. I feel, it is not
required to mention the Note section.

+/* Ignore vacuum backends and analyze ones */

How about changing it as "Ignore both vacuum and analyze backends".


Regards,
Hari Babu
Fujitsu Australia
On 2017-03-21 07:46:47 Haribabu Kommi wrote:
>On Tue, Mar 21, 2017 at 3:16 PM, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:
>+/* Use these flags in GetOldestXmin as "flags" */
>
>How about some thing like the following.
>/* Use the following flags as an input "flags" to GetOldestXmin function */
>
>
>+/* Ignore vacuum backends (Note: this also ignores analyze with vacuum backends) */
>+#define        PROCARRAY_FLAGS_VACUUM            PROCARRAY_FLAGS_DEFAULT | PROCARRAY_VACUUM_FLAG
>+/* Ignore analyze backends (Note: this also ignores vacuum with analyze backends) */
>+#define        PROCARRAY_FLAGS_ANALYZE            PROCARRAY_FLAGS_DEFAULT | PROCARRAY_ANALYZE_FLAG
>
>Whenever the above flags are passed to the GetOldestXmin() function,
>it just verifies whether any one of the flags are set in the backend flags
>or not. And also actually the PROC_IN_ANALYZE flag will set when
>analyze operation is started and reset at the end. I feel, it is not
>required to mention the Note section.
>
>+/* Ignore vacuum backends and analyze ones */
>
>How about changing it as "Ignore both vacuum and analyze backends".

Thank you for your review, again.

I think your proposals are better, so I reflected them.

--
Regards,
Eiji Seki
Fujitsu



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

Attachment

Re: [HACKERS] Proposal: GetOldestXminExtend for ignoring arbitraryvacuum flags

From
Haribabu Kommi
Date:


On Wed, Mar 22, 2017 at 1:53 PM, Seki, Eiji <seki.eiji@jp.fujitsu.com> wrote:

Thank you for your review, again.

I think your proposals are better, so I reflected them.

Thanks for the updated patch. Patch looks good to me.
I marked it as "ready for committer".

While reviewing this patch, I found that PGXACT->vacuumFlags
variable name needs a rename because with the addition of
PROC_IN_LOGICAL_DECODING flag "vacuumFlags" doesn't
only use it for vacuum operation. I feel this variable can be renamed
as just "flags", but anyway that is a different patch.

Regards,
Hari Babu
Fujitsu Australia
On 22 March 2017 at 03:42, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>
>
> On Wed, Mar 22, 2017 at 1:53 PM, Seki, Eiji <seki.eiji@jp.fujitsu.com>
> wrote:
>>
>>
>> Thank you for your review, again.
>>
>> I think your proposals are better, so I reflected them.
>
>
> Thanks for the updated patch. Patch looks good to me.
> I marked it as "ready for committer".

Looks good. I'll double check and commit this.

> While reviewing this patch, I found that PGXACT->vacuumFlags
> variable name needs a rename because with the addition of
> PROC_IN_LOGICAL_DECODING flag "vacuumFlags" doesn't
> only use it for vacuum operation. I feel this variable can be renamed
> as just "flags", but anyway that is a different patch.

Good point. Should be an open item.

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