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

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

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

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

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)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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