Обсуждение: Cleanup isolation specs from unused steps
Hi all, I have been looking at the isolation tests, and we have in some specs steps which are defined but not used in any permutations. In order to detect them, I have been using the attached trick to track which permutations are used. This allows to find immediately any over-engineered spec by generating diffs about steps defined by not used in any permutations. On HEAD, we have six specs entering in this category. Would that be useful? -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes:
> I have been looking at the isolation tests, and we have in some specs
> steps which are defined but not used in any permutations.
Hmm, might any of those represent actual bugs? Or are they just
leftovers from test development?
> In order to
> detect them, I have been using the attached trick to track which
> permutations are used. This allows to find immediately any
> over-engineered spec by generating diffs about steps defined by not
> used in any permutations. On HEAD, we have six specs entering in this
> category.
Seems like a good idea; I'm surprised we've got so many cases.
regards, tom lane
On Mon, Aug 19, 2019 at 1:08 AM Michael Paquier <michael@paquier.xyz> wrote:
Hi all,
I have been looking at the isolation tests, and we have in some specs
steps which are defined but not used in any permutations. In order to
detect them, I have been using the attached trick to track which
permutations are used. This allows to find immediately any
over-engineered spec by generating diffs about steps defined by not
used in any permutations. On HEAD, we have six specs entering in this
category.
Would that be useful?
I think it is useful.
could you do the check that all steps have been used in dry_run mode
instead of when running the tests for real?
instead of when running the tests for real?
--
Melanie Plageman
On Mon, Aug 19, 2019 at 11:02:42AM -0400, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: >> I have been looking at the isolation tests, and we have in some specs >> steps which are defined but not used in any permutations. > > Hmm, might any of those represent actual bugs? Or are they just > leftovers from test development? I cannot yet enter the minds of each test author back this much in time, but I think that's a mix of both. When working on a new isolation spec, I personally tend to do a lot of copy-pasting of the same queries for multiple sessions and then manipulate the permutations to produce a set of useful tests. It is rather easy to forget to remove some steps when doing that. I guess that's what happened with tuplelock-upgrade, insert-conflict-do-update* and freeze-the-dead. -- Michael
Вложения
On Mon, Aug 19, 2019 at 10:23:19AM -0700, Melanie Plageman wrote: > Could you do the check that all steps have been used in dry_run mode > instead of when running the tests for real? Sure, I was hesitating to do so. I have no issue in moving the check into run_testspec(). So done as attached. It is rather a pain to pass down custom options to isolationtester. For example, I have tested the updated version attached after hijacking -n into isolation_start_test(). Ugly hack, but for testing that's enough. Do you make use of this tool in a particular way in greenplum? Just wondering. (Could it make sense to have long options for isolationtester by the way?) -- Michael
Вложения
On 2019-Aug-20, Michael Paquier wrote: > On Mon, Aug 19, 2019 at 10:23:19AM -0700, Melanie Plageman wrote: > > Could you do the check that all steps have been used in dry_run mode > > instead of when running the tests for real? > > Sure, I was hesitating to do so. I have no issue in moving the check > into run_testspec(). So done as attached. I created the dry-run mode to be able to easily generate the set of possible permutations for a new test, then edit the result and put it back in the spec file; but after the deadlock tests were added (with necessary hacking of the lock-detection in isolationtester) that manner of operation became almost completely useless. Maybe we need to rethink what facilities isolationtester offers -- possibly making dry-run have a completely different behavior than currently, which I doubt anybody is using. All that being said, I have no objections to this patch (but I didn't review it closely). -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Aug 20, 2019 at 12:34:45AM -0400, Alvaro Herrera wrote: > I created the dry-run mode to be able to easily generate the set of > possible permutations for a new test, then edit the result and put it > back in the spec file; but after the deadlock tests were added (with > necessary hacking of the lock-detection in isolationtester) that manner > of operation became almost completely useless. Maybe we need to rethink > what facilities isolationtester offers -- possibly making dry-run have a > completely different behavior than currently, which I doubt anybody is > using. I am not sure exactly how it could be redesigned, and with n! permutations that easily leads to bloat of the generated output. I think that --dry-run (well -n) is a bit misleading as option name though as it prints only permutations. Still, keeping it around has no real cost, so it is not a big deal. (Looking at the gpdb code, it does not seem to be used.) > All that being said, I have no objections to this patch (but I didn't > review it closely). Thanks. -- Michael
Вложения
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Aug-20, Michael Paquier wrote:
>> On Mon, Aug 19, 2019 at 10:23:19AM -0700, Melanie Plageman wrote:
>>> Could you do the check that all steps have been used in dry_run mode
>>> instead of when running the tests for real?
>> Sure, I was hesitating to do so. I have no issue in moving the check
>> into run_testspec(). So done as attached.
> I created the dry-run mode to be able to easily generate the set of
> possible permutations for a new test, then edit the result and put it
> back in the spec file; but after the deadlock tests were added (with
> necessary hacking of the lock-detection in isolationtester) that manner
> of operation became almost completely useless. Maybe we need to rethink
> what facilities isolationtester offers -- possibly making dry-run have a
> completely different behavior than currently, which I doubt anybody is
> using.
Hm, does that mean that this version of the patch would fail to warn
during a normal run? Doesn't sound good, since as Alvaro says,
hardly anyone uses dry-run.
If you can warn in both cases, that'd be OK perhaps. But Alvaro's
description of the intended use of dry-run makes it sound like
it would be expected for there to be unreferenced steps, since there'd
be no permutations yet in the input.
regards, tom lane
On 2019-Aug-20, Tom Lane wrote: > If you can warn in both cases, that'd be OK perhaps. But Alvaro's > description of the intended use of dry-run makes it sound like > it would be expected for there to be unreferenced steps, since there'd > be no permutations yet in the input. Well, Heikki/Kevin's original intention was that if no permutations are specified, then all possible permutations are generated internally and the spec is run with that. The intended use of dry-run was to do just that (generate all possible permutations) and print that list, so that it could be trimmed down by the test author. In this mode of operation, all steps are always used, so there'd be no warning printed. However, when a test file has a largish number of steps then the list is, um, a bit long. Before the deadlock-test hacking, you could run with such a list anyway and any permutations that caused a blockage would be reported right away as an invalid permutation -- quick enough. Currently it sleeps for absurdly long on those cases, so this is no longer feasible. This is why I say that the current dry-run mode could be removed with no loss of useful functionality. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Aug 20, 2019 at 09:54:56AM -0400, Alvaro Herrera wrote: > On 2019-Aug-20, Tom Lane wrote: >> If you can warn in both cases, that'd be OK perhaps. But Alvaro's >> description of the intended use of dry-run makes it sound like >> it would be expected for there to be unreferenced steps, since there'd >> be no permutations yet in the input. v2 of the patch warns of any unused steps in dry-run mode. If no permutations are defined in the input spec, all steps get used (actually that's not a factorial as the per-session ordering is preserved), so I would expect no warnings to be generated and the patch does that. If the test includes some permutations, then I would expect dry-run to complain about the steps which are defined in the spec but not used. The patch also does that. Do you see a problem with that? > Well, Heikki/Kevin's original intention was that if no permutations are > specified, then all possible permutations are generated internally and > the spec is run with that. The intended use of dry-run was to do just > that (generate all possible permutations) and print that list, so that > it could be trimmed down by the test author. In this mode of operation, > all steps are always used, so there'd be no warning printed. However, > when a test file has a largish number of steps then the list is, um, a > bit long. Before the deadlock-test hacking, you could run with such a > list anyway and any permutations that caused a blockage would be > reported right away as an invalid permutation -- quick enough. > Currently it sleeps for absurdly long on those cases, so this is no > longer feasible. > > This is why I say that the current dry-run mode could be removed with no > loss of useful functionality. Hmm. Even if one does not do something deadlock-specific, the list printed could still be useful, no? This for example works now that I look at it: ./isolationtester -n < specs/my_spec.spec I am wondering if we should not actually keep dry_run, but rename it to something like --print-permutations to print the set of permutations which would be run as part of the spec, and also have an option which is able to print out all permutations possible, like --print-all-permutations. Simply ripping out the mode would be fine by me as it does not seem to be used, but keeping it around does not induce really much extra maintenance cost. -- Michael
Вложения
On Mon, Aug 19, 2019 at 7:01 PM Michael Paquier <michael@paquier.xyz> wrote:
It is rather a pain to pass down custom options to isolationtester.
For example, I have tested the updated version attached after
hijacking -n into isolation_start_test(). Ugly hack, but for testing
that's enough. Do you make use of this tool in a particular way in
greenplum? Just wondering.
(Could it make sense to have long options for isolationtester by the
way?)
In Greenplum, we mainly add new tests to a separate isolation
framework (called isolation2) which uses a completely different
syntax. It doesn't use isolationtester at all. So, I haven't had a use
case to add long options to isolationtester yet :)
framework (called isolation2) which uses a completely different
syntax. It doesn't use isolationtester at all. So, I haven't had a use
case to add long options to isolationtester yet :)
--
Melanie Plageman
On Tue, Aug 20, 2019 at 6:34 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Aug 20, 2019 at 09:54:56AM -0400, Alvaro Herrera wrote:
> On 2019-Aug-20, Tom Lane wrote:
>> If you can warn in both cases, that'd be OK perhaps. But Alvaro's
>> description of the intended use of dry-run makes it sound like
>> it would be expected for there to be unreferenced steps, since there'd
>> be no permutations yet in the input.
v2 of the patch warns of any unused steps in dry-run mode. If no
permutations are defined in the input spec, all steps get used
(actually that's not a factorial as the per-session ordering is
preserved), so I would expect no warnings to be generated and the
patch does that. If the test includes some permutations, then I would
expect dry-run to complain about the steps which are defined in the
spec but not used. The patch also does that. Do you see a problem
with that?
> Well, Heikki/Kevin's original intention was that if no permutations are
> specified, then all possible permutations are generated internally and
> the spec is run with that. The intended use of dry-run was to do just
> that (generate all possible permutations) and print that list, so that
> it could be trimmed down by the test author. In this mode of operation,
> all steps are always used, so there'd be no warning printed. However,
> when a test file has a largish number of steps then the list is, um, a
> bit long. Before the deadlock-test hacking, you could run with such a
> list anyway and any permutations that caused a blockage would be
> reported right away as an invalid permutation -- quick enough.
> Currently it sleeps for absurdly long on those cases, so this is no
> longer feasible.
>
> This is why I say that the current dry-run mode could be removed with no
> loss of useful functionality.
Hmm. Even if one does not do something deadlock-specific, the list
printed could still be useful, no? This for example works now that I
look at it:
./isolationtester -n < specs/my_spec.spec
I am wondering if we should not actually keep dry_run, but rename it
to something like --print-permutations to print the set of
permutations which would be run as part of the spec, and also have an
option which is able to print out all permutations possible, like
--print-all-permutations. Simply ripping out the mode would be fine
by me as it does not seem to be used, but keeping it around does not
induce really much extra maintenance cost.
So, I think I completely misunderstood the purpose of 'dry-run'. If no
one is using it, having a check for unused steps in dry-run may not be
useful.
+1 to renaming it to --print-permutations and, potentially,
adding --print-all-permutations
one is using it, having a check for unused steps in dry-run may not be
useful.
+1 to renaming it to --print-permutations and, potentially,
adding --print-all-permutations
--
Melanie Plageman
On 2019-Aug-21, Melanie Plageman wrote: > In Greenplum, we mainly add new tests to a separate isolation > framework (called isolation2) which uses a completely different > syntax. It doesn't use isolationtester at all. So, I haven't had a use > case to add long options to isolationtester yet :) Is that other framework somehow more capable? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Aug 21, 2019 at 11:07:19AM -0700, Melanie Plageman wrote: > So, I think I completely misunderstood the purpose of 'dry-run'. If no > one is using it, having a check for unused steps in dry-run may not be > useful. Okay. After sleeping on it and seeing how this thread evolves, it looks that we have more arguments in favor of just let dry-run go to the void. So attached is an updated patch set: - 0001 removes the dry-run mode from isolationtester. - 0002 cleans up the specs of unused steps and adds the discussed sanity checks, as proposed for this thread. -- Michael
Вложения
On Wed, Aug 21, 2019 at 12:16 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-Aug-21, Melanie Plageman wrote:
> In Greenplum, we mainly add new tests to a separate isolation
> framework (called isolation2) which uses a completely different
> syntax. It doesn't use isolationtester at all. So, I haven't had a use
> case to add long options to isolationtester yet :)
Is that other framework somehow more capable?
So, there is some historical context as to why it is a separate test suite.
And some of the differences are specific to Greenplum -- e.g. needing to connect
to a specific database in "utility mode" to do something.
However, the other differences are actually pretty handy and would be applicable
to upstream as well.
We use a different syntax than the isolation framework and have some nice
features. Most notably, explicit control over blocking.
The syntax for what would be a "step" in isolation is like this:
[<#>[flag]:] <sql> | ! <shell scripts or command>
where # is the session number and flags include the following:
&: expect blocking behavior
>: running in background without blocking
<: join an existing session
q: quit the given session
See the script [1] for parsing the test cases for more details on the syntax and
capabilities (it is in Python).
[1] https://github.com/greenplum-db/gpdb/blob/master/src/test/isolation2/sql_isolation_testcase.py
And some of the differences are specific to Greenplum -- e.g. needing to connect
to a specific database in "utility mode" to do something.
However, the other differences are actually pretty handy and would be applicable
to upstream as well.
We use a different syntax than the isolation framework and have some nice
features. Most notably, explicit control over blocking.
The syntax for what would be a "step" in isolation is like this:
[<#>[flag]:] <sql> | ! <shell scripts or command>
where # is the session number and flags include the following:
&: expect blocking behavior
>: running in background without blocking
<: join an existing session
q: quit the given session
See the script [1] for parsing the test cases for more details on the syntax and
capabilities (it is in Python).
[1] https://github.com/greenplum-db/gpdb/blob/master/src/test/isolation2/sql_isolation_testcase.py
--
Melanie Plageman
On Thu, Aug 22, 2019 at 1:45 PM Melanie Plageman <melanieplageman@gmail.com> wrote: > > > On Wed, Aug 21, 2019 at 12:16 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> >> On 2019-Aug-21, Melanie Plageman wrote: >> >> > In Greenplum, we mainly add new tests to a separate isolation >> > framework (called isolation2) which uses a completely different >> > syntax. It doesn't use isolationtester at all. So, I haven't had a use >> > case to add long options to isolationtester yet :) >> >> Is that other framework somehow more capable? > > > So, there is some historical context as to why it is a separate test suite. > And some of the differences are specific to Greenplum -- e.g. needing to connect > to a specific database in "utility mode" to do something. > > However, the other differences are actually pretty handy and would be applicable > to upstream as well. > We use a different syntax than the isolation framework and have some nice > features. Most notably, explicit control over blocking. Asim submitted this framework just yesterday: https://www.postgresql.org/message-id/CANXE4TdxdESX1jKw48xet-5GvBFVSq=4cgNeioTQff372KO45A@mail.gmail.com -- Rob > > The syntax for what would be a "step" in isolation is like this: > > [<#>[flag]:] <sql> | ! <shell scripts or command> > > where # is the session number and flags include the following: > > &: expect blocking behavior > >: running in background without blocking > <: join an existing session > q: quit the given session > > See the script [1] for parsing the test cases for more details on the syntax and > capabilities (it is in Python). > > [1] https://github.com/greenplum-db/gpdb/blob/master/src/test/isolation2/sql_isolation_testcase.py > > -- > Melanie Plageman
On Thu, Aug 22, 2019 at 10:20:48AM -0700, Melanie Plageman wrote: > So, there is some historical context as to why it is a separate test suite. > And some of the differences are specific to Greenplum -- e.g. needing to > connect to a specific database in "utility mode" to do something. What is "utility mode"? > The syntax for what would be a "step" in isolation is like this: > > [<#>[flag]:] <sql> | ! <shell scripts or command> > > where # is the session number and flags include the following: > > &: expect blocking behavior > >: running in background without blocking > <: join an existing session > q: quit the given session These could be transposed as new meta commands for the existing specs? Of course not as "step" per-se, but new dedicated commands? > See the script [1] for parsing the test cases for more details on the > syntax and capabilities (it is in Python). Hmm. The bar to add a new hard language dependency in the test suites is very high. I am not sure that we'd want something with a python dependency for the tests, also knowing how Python likes breaking compatibility (isolation2_main() also mentions a dependency to Python). -- Michael
Вложения
On Thu, Aug 22, 2019 at 6:53 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Aug 22, 2019 at 10:20:48AM -0700, Melanie Plageman wrote:
> So, there is some historical context as to why it is a separate test suite.
> And some of the differences are specific to Greenplum -- e.g. needing to
> connect to a specific database in "utility mode" to do something.
What is "utility mode"?
It's a connection parameter that allows you to connect to a single Postgres node
in a Greenplum cluster. I only included it as an example of the kind of
"Greenplum-specific" things that are in the test framework.
in a Greenplum cluster. I only included it as an example of the kind of
"Greenplum-specific" things that are in the test framework.
> The syntax for what would be a "step" in isolation is like this:
>
> [<#>[flag]:] <sql> | ! <shell scripts or command>
>
> where # is the session number and flags include the following:
>
> &: expect blocking behavior
> >: running in background without blocking
> <: join an existing session
> q: quit the given session
These could be transposed as new meta commands for the existing
specs? Of course not as "step" per-se, but new dedicated commands?
Yes, I think you could definitely add some of the flags as meta-commands for
existing steps -- the current implementation of "blocking" for isolation is
really limiting.
However, features aside, I actually find the existing "step" syntax in isolation
clunkier than the syntax used in Greenplum's "isolation2" framework.
existing steps -- the current implementation of "blocking" for isolation is
really limiting.
However, features aside, I actually find the existing "step" syntax in isolation
clunkier than the syntax used in Greenplum's "isolation2" framework.
> See the script [1] for parsing the test cases for more details on the
> syntax and capabilities (it is in Python).
Hmm. The bar to add a new hard language dependency in the test
suites is very high. I am not sure that we'd want something with a
python dependency for the tests, also knowing how Python likes
breaking compatibility (isolation2_main() also mentions a dependency
to Python).
Agreed, I don't think it needs to be in Python.
My point was that some of our "isolation2" framework has to be different because
it is enabling us to test features that are in Greenplum and not in Postgres.
However, many of the features it has would actually be really handy to have for
testing Postgres.
It wasn't initially suggested upstream because it is actually mainly ported from
a separate standalone testing framework that was written at Greenplum in Python.
I've heard Greenplum folks talk about re-writing our "isolation2" framework in
(probably) C and making it a better fit to contribute. It's definitely on my wishlist.
it is enabling us to test features that are in Greenplum and not in Postgres.
However, many of the features it has would actually be really handy to have for
testing Postgres.
It wasn't initially suggested upstream because it is actually mainly ported from
a separate standalone testing framework that was written at Greenplum in Python.
I've heard Greenplum folks talk about re-writing our "isolation2" framework in
(probably) C and making it a better fit to contribute. It's definitely on my wishlist.
Melanie Plageman
On Thu, Aug 22, 2019 at 09:19:47PM -0700, Melanie Plageman wrote: > It's a connection parameter that allows you to connect to a single Postgres > node in a Greenplum cluster. I only included it as an example of the kind of > "Greenplum-specific" things that are in the test framework. Ah, I see. I had the same stuff for XC to connect to data nodes. > I've heard Greenplum folks talk about re-writing our "isolation2" framework > in (probably) C and making it a better fit to contribute. It's definitely on > my wishlist. Nice to hear that. -- Michael
Вложения
On Thu, Aug 22, 2019 at 12:47 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> On 2019-Aug-21, Melanie Plageman wrote:
>
> > In Greenplum, we mainly add new tests to a separate isolation
> > framework (called isolation2) which uses a completely different
> > syntax. It doesn't use isolationtester at all. So, I haven't had a use
> > case to add long options to isolationtester yet :)
>
> Is that other framework somehow more capable?
>
The ability to declare a step as blocking, as Melanie mentioned upthread ("&" prefix), makes it more capable. The tester, when encounters such a step, makes sure that the command in that step is blocking and moves on to run subsequent commands. The isolationtester, on the other hand, treats a step as blocking only when the command waits on a lock. That seems restrictive. E.g. what if a command waits on a latch, as part of a valid interleaving of concurrent transactions? The isolation tester cannot detect such a case and it will keep waiting and eventually fail the test with a timeout.
As part of the fault injector patch set [1], I added a new "blocking" keyword to isolation grammar so that a step can be declared as blocking. See patch 0002-Add-syntax-to-declare-a-step-that-is-expected-to-block.
>
> On 2019-Aug-21, Melanie Plageman wrote:
>
> > In Greenplum, we mainly add new tests to a separate isolation
> > framework (called isolation2) which uses a completely different
> > syntax. It doesn't use isolationtester at all. So, I haven't had a use
> > case to add long options to isolationtester yet :)
>
> Is that other framework somehow more capable?
>
The ability to declare a step as blocking, as Melanie mentioned upthread ("&" prefix), makes it more capable. The tester, when encounters such a step, makes sure that the command in that step is blocking and moves on to run subsequent commands. The isolationtester, on the other hand, treats a step as blocking only when the command waits on a lock. That seems restrictive. E.g. what if a command waits on a latch, as part of a valid interleaving of concurrent transactions? The isolation tester cannot detect such a case and it will keep waiting and eventually fail the test with a timeout.
As part of the fault injector patch set [1], I added a new "blocking" keyword to isolation grammar so that a step can be declared as blocking. See patch 0002-Add-syntax-to-declare-a-step-that-is-expected-to-block.
Asim
On 2019-Aug-23, Asim R P wrote: > As part of the fault injector patch set [1], I added a new "blocking" > keyword to isolation grammar so that a step can be declared as blocking. > See patch 0002-Add-syntax-to-declare-a-step-that-is-expected-to-block. One point to that implementation is that in that design a step is globally declared to be blocking, but in reality that's the wrong way to see things: a step might block in some permutations and not others. So I think we should do as Michael suggested: it's the permutation that has to have a way to mark a given step as blocking, not the step itself. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
(Re-adding pgsql-hackers) On Fri, Aug 23, 2019 at 05:28:35PM +0530, Asim R P wrote: > Both the patches look good to me, thank you. +1 to removing dry-run and > tracking unused steps. Thanks, both have been committed. There have been issues with the isolation tests of logical decoding I have taken care of. -- Michael
Вложения
On Fri, Aug 23, 2019 at 9:08 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> On 2019-Aug-23, Asim R P wrote:
>
> > As part of the fault injector patch set [1], I added a new "blocking"
> > keyword to isolation grammar so that a step can be declared as blocking.
> > See patch 0002-Add-syntax-to-declare-a-step-that-is-expected-to-block.
>
> One point to that implementation is that in that design a step is
> globally declared to be blocking, but in reality that's the wrong way to
> see things: a step might block in some permutations and not others. So
> I think we should do as Michael suggested: it's the permutation that has
> to have a way to mark a given step as blocking, not the step itself.
>
> On 2019-Aug-23, Asim R P wrote:
>
> > As part of the fault injector patch set [1], I added a new "blocking"
> > keyword to isolation grammar so that a step can be declared as blocking.
> > See patch 0002-Add-syntax-to-declare-a-step-that-is-expected-to-block.
>
> One point to that implementation is that in that design a step is
> globally declared to be blocking, but in reality that's the wrong way to
> see things: a step might block in some permutations and not others. So
> I think we should do as Michael suggested: it's the permutation that has
> to have a way to mark a given step as blocking, not the step itself.
Thank you for the feedback. I've changed patch 0002 accordingly, please take another look: https://www.postgresql.org/message-id/CANXE4TdvSi7Yia_5sV82%2BMHf0WcUSN9u6_X8VEUBv-YStphd%3DQ%40mail.gmail.com
Asim
On Tue, Aug 27, 2019 at 07:05:50PM +0530, Asim R P wrote: > Thank you for the feedback. I've changed patch 0002 accordingly, please > take another look: > https://www.postgresql.org/message-id/CANXE4TdvSi7Yia_5sV82%2BMHf0WcUSN9u6_X8VEUBv-YStphd%3DQ%40mail.gmail.com Thanks! Let's move the discussion on the other thread then. -- Michael