Re: [HACKERS] pg_replication_origin_xact_reset() and its argument variables

[Fujii Masao]

On Tue, Aug 2, 2016 at 2:59 AM, Fujii Masao  wrote:
> On Tue, Aug 2, 2016 at 2:48 AM, Andres Freund  wrote:
>> Hi Fujii,
>>
>> On 2016-07-28 16:44:37 +0900, Fujii Masao wrote:
>>> On Sat, Jul 2, 2016 at 7:01 AM, Tom Lane  wrote:
>>> > Andres Freund  writes:
>>> >> On 2016-06-30 10:14:04 -0400, Tom Lane wrote:
>>> >>> Fujii Masao  writes:
>>>  As far as I read the code of the function, those arguments don't seem 
>>>  to
>>>  be necessary. So I'm afraid that the pg_proc entry for the function 
>>>  might
>>>  be incorrect and those two arguments should be removed from the 
>>>  definition.
>>> >
>>> >>> Sure looks that way from here.  Copy-and-paste from the previous
>>> >>> line in pg_proc.h, perhaps?
>>> >
>>> >> Yes, that's clearly wrong.
>>>
>>> Attached patch (pg_replication_origin_xact_reset_9.6.patch) fixes this.
>>> We need to apply this at least before RC1 of PostgreSQL9.6 will be released
>>> because the patch needs the change of catalog version.
>>>
>>> >> Damn.  Can't fix that for 9.5 anymore. The
>>> >> function isn't all that important (especially not from SQL), but still,
>>> >> that's annoying.  I'm inclined to just remove the args in 9.6. We could
>>> >> also add a note to the 9.5 docs, adding that the arguments are there by
>>> >> error?
>>>
>>> What about the attched patch (pg_replication_origin_xact_reset_9.5.patch)?
>>
>> except for the strictness remark in the other email,
>
> Yes, you're right. My careless mistake... :(
>
>> these look sane to
>> me.  Do you want to push them?  I'll do so by Wednesday otherwise, to
>> leave some room before the next RC.
>
> Could you do that if possible?

Pushed since right now I have time to do that. Anyway, thanks!

Regards,

-- 
Fujii Masao


-- 
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_replication_origin_xact_reset() and its argument variables

[Fujii Masao]

On Tue, Aug 2, 2016 at 2:48 AM, Andres Freund  wrote:
> Hi Fujii,
>
> On 2016-07-28 16:44:37 +0900, Fujii Masao wrote:
>> On Sat, Jul 2, 2016 at 7:01 AM, Tom Lane  wrote:
>> > Andres Freund  writes:
>> >> On 2016-06-30 10:14:04 -0400, Tom Lane wrote:
>> >>> Fujii Masao  writes:
>>  As far as I read the code of the function, those arguments don't seem to
>>  be necessary. So I'm afraid that the pg_proc entry for the function 
>>  might
>>  be incorrect and those two arguments should be removed from the 
>>  definition.
>> >
>> >>> Sure looks that way from here.  Copy-and-paste from the previous
>> >>> line in pg_proc.h, perhaps?
>> >
>> >> Yes, that's clearly wrong.
>>
>> Attached patch (pg_replication_origin_xact_reset_9.6.patch) fixes this.
>> We need to apply this at least before RC1 of PostgreSQL9.6 will be released
>> because the patch needs the change of catalog version.
>>
>> >> Damn.  Can't fix that for 9.5 anymore. The
>> >> function isn't all that important (especially not from SQL), but still,
>> >> that's annoying.  I'm inclined to just remove the args in 9.6. We could
>> >> also add a note to the 9.5 docs, adding that the arguments are there by
>> >> error?
>>
>> What about the attched patch (pg_replication_origin_xact_reset_9.5.patch)?
>
> except for the strictness remark in the other email,

Yes, you're right. My careless mistake... :(

> these look sane to
> me.  Do you want to push them?  I'll do so by Wednesday otherwise, to
> leave some room before the next RC.

Could you do that if possible?

Regards,

-- 
Fujii Masao


-- 
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_replication_origin_xact_reset() and its argument variables

[Andres Freund]

Hi Fujii,

On 2016-07-28 16:44:37 +0900, Fujii Masao wrote:
> On Sat, Jul 2, 2016 at 7:01 AM, Tom Lane  wrote:
> > Andres Freund  writes:
> >> On 2016-06-30 10:14:04 -0400, Tom Lane wrote:
> >>> Fujii Masao  writes:
>  As far as I read the code of the function, those arguments don't seem to
>  be necessary. So I'm afraid that the pg_proc entry for the function might
>  be incorrect and those two arguments should be removed from the 
>  definition.
> >
> >>> Sure looks that way from here.  Copy-and-paste from the previous
> >>> line in pg_proc.h, perhaps?
> >
> >> Yes, that's clearly wrong.
> 
> Attached patch (pg_replication_origin_xact_reset_9.6.patch) fixes this.
> We need to apply this at least before RC1 of PostgreSQL9.6 will be released
> because the patch needs the change of catalog version.
> 
> >> Damn.  Can't fix that for 9.5 anymore. The
> >> function isn't all that important (especially not from SQL), but still,
> >> that's annoying.  I'm inclined to just remove the args in 9.6. We could
> >> also add a note to the 9.5 docs, adding that the arguments are there by
> >> error?
> 
> What about the attched patch (pg_replication_origin_xact_reset_9.5.patch)?

except for the strictness remark in the other email, these look sane to
me.  Do you want to push them?  I'll do so by Wednesday otherwise, to
leave some room before the next RC.

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: [HACKERS] pg_replication_origin_xact_reset() and its argument variables

[Andres Freund]

On 2016-07-28 16:44:37 +0900, Fujii Masao wrote:
> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> index d6ed0ce..0a3d3de 100644
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -17519,7 +17519,7 @@ postgres=# SELECT * FROM 
> pg_xlogfile_name_offset(pg_stop_backup());
>  
>   pg_replication_origin_xact_reset
>  
> -
> pg_replication_origin_xact_reset()
> +
> pg_replication_origin_xact_reset(origin_lsn
>  pg_lsn, origin_timestamp 
> timestamptz)
> 
> 
>  void
> @@ -17527,6 +17527,12 @@ postgres=# SELECT * FROM 
> pg_xlogfile_name_offset(pg_stop_backup());
> 
>  Cancel the effects of
>  pg_replication_origin_xact_setup().
> +Note that two arguments were introduced by mistake
> +during the PostgreSQL 9.5 development cycle while
> +pg_replication_origin_xact_reset() actually
> +doesn't use them at all. Therefore, any dummy values like
> +NULL can be safely specified as the arguments.
> +This mistake will be fixed in a future release.
> 
>

I don't think NULL works, the function is marked as strict. Otherwise
that looks right.

Thanks,

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_replication_origin_xact_reset() and its argument variables

[Robert Haas]

On Thu, Jul 28, 2016 at 3:44 AM, Fujii Masao  wrote:
 Sure looks that way from here.  Copy-and-paste from the previous
 line in pg_proc.h, perhaps?
>>
>>> Yes, that's clearly wrong.
>
> Attached patch (pg_replication_origin_xact_reset_9.6.patch) fixes this.
> We need to apply this at least before RC1 of PostgreSQL9.6 will be released
> because the patch needs the change of catalog version.
>
>>> Damn.  Can't fix that for 9.5 anymore. The
>>> function isn't all that important (especially not from SQL), but still,
>>> that's annoying.  I'm inclined to just remove the args in 9.6. We could
>>> also add a note to the 9.5 docs, adding that the arguments are there by
>>> error?
>
> What about the attched patch (pg_replication_origin_xact_reset_9.5.patch)?

I think you should apply these ASAP.

-- 
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_replication_origin_xact_reset() and its argument variables

[Fujii Masao]

On Sat, Jul 2, 2016 at 7:01 AM, Tom Lane  wrote:
> Andres Freund  writes:
>> On 2016-06-30 10:14:04 -0400, Tom Lane wrote:
>>> Fujii Masao  writes:
 As far as I read the code of the function, those arguments don't seem to
 be necessary. So I'm afraid that the pg_proc entry for the function might
 be incorrect and those two arguments should be removed from the definition.
>
>>> Sure looks that way from here.  Copy-and-paste from the previous
>>> line in pg_proc.h, perhaps?
>
>> Yes, that's clearly wrong.

Attached patch (pg_replication_origin_xact_reset_9.6.patch) fixes this.
We need to apply this at least before RC1 of PostgreSQL9.6 will be released
because the patch needs the change of catalog version.

>> Damn.  Can't fix that for 9.5 anymore. The
>> function isn't all that important (especially not from SQL), but still,
>> that's annoying.  I'm inclined to just remove the args in 9.6. We could
>> also add a note to the 9.5 docs, adding that the arguments are there by
>> error?

What about the attched patch (pg_replication_origin_xact_reset_9.5.patch)?

Regards,

-- 
Fujii Masao
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index d6ed0ce..0a3d3de 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17519,7 +17519,7 @@ postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
 
  pg_replication_origin_xact_reset
 
-pg_replication_origin_xact_reset()
+pg_replication_origin_xact_reset(origin_lsn pg_lsn, origin_timestamp timestamptz)


 void
@@ -17527,6 +17527,12 @@ postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());

 Cancel the effects of
 pg_replication_origin_xact_setup().
+Note that two arguments were introduced by mistake
+during the PostgreSQL 9.5 development cycle while
+pg_replication_origin_xact_reset() actually
+doesn't use them at all. Therefore, any dummy values like
+NULL can be safely specified as the arguments.
+This mistake will be fixed in a future release.

   
 
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 4fd532a..34735fb 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*			mmddN */
-#define CATALOG_VERSION_NO	201607071
+#define CATALOG_VERSION_NO	201607281
 
 #endif
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 5d233e3..270dd21 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -5284,7 +5284,7 @@ DESCR("get the replication progress of the current session");
 DATA(insert OID = 6010 ( pg_replication_origin_xact_setup PGNSP PGUID 12 1 0 0 0 f f f f t f v r 2 0 2278 "3220 1184" _null_ _null_ _null_ _null_ _null_ pg_replication_origin_xact_setup _null_ _null_ _null_ ));
 DESCR("setup the transaction's origin lsn and timestamp");
 
-DATA(insert OID = 6011 ( pg_replication_origin_xact_reset PGNSP PGUID 12 1 0 0 0 f f f f t f v r 2 0 2278 "3220 1184" _null_ _null_ _null_ _null_ _null_ pg_replication_origin_xact_reset _null_ _null_ _null_ ));
+DATA(insert OID = 6011 ( pg_replication_origin_xact_reset PGNSP PGUID 12 1 0 0 0 f f f f t f v r 0 0 2278 "" _null_ _null_ _null_ _null_ _null_ pg_replication_origin_xact_reset _null_ _null_ _null_ ));
 DESCR("reset the transaction's origin lsn and timestamp");
 
 DATA(insert OID = 6012 ( pg_replication_origin_advance PGNSP PGUID 12 1 0 0 0 f f f f t f v u 2 0 2278 "25 3220" _null_ _null_ _null_ _null_ _null_ pg_replication_origin_advance _null_ _null_ _null_ ));

-- 
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_replication_origin_xact_reset() and its argument variables

[Tom Lane]

Andres Freund  writes:
> On 2016-06-30 10:14:04 -0400, Tom Lane wrote:
>> Fujii Masao  writes:
>>> As far as I read the code of the function, those arguments don't seem to
>>> be necessary. So I'm afraid that the pg_proc entry for the function might
>>> be incorrect and those two arguments should be removed from the definition.

>> Sure looks that way from here.  Copy-and-paste from the previous
>> line in pg_proc.h, perhaps?

> Yes, that's clearly wrong. Damn.  Can't fix that for 9.5 anymore. The
> function isn't all that important (especially not from SQL), but still,
> that's annoying.  I'm inclined to just remove the args in 9.6. We could
> also add a note to the 9.5 docs, adding that the arguments are there by
> error?

Yeah, seems like the best thing to do.

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_replication_origin_xact_reset() and its argument variables

[Andres Freund]

On 2016-06-30 10:14:04 -0400, Tom Lane wrote:
> Fujii Masao  writes:
> > The document explains that pg_replication_origin_xact_reset() doesn't have
> > any argument variables. But it's been actually defined so as to have two
> > argument variables with pg_lsn and timestamptz data types, as follows.
> 
> > =# \df pg_replication_origin_xact_reset
> >   List of functions
> >Schema   |   Name   | Result data type |
> >Argument data types|  Type
> > +--+--+--+
> >  pg_catalog | pg_replication_origin_xact_reset | void |
> > pg_lsn, timestamp with time zone | normal
> 
> > As far as I read the code of the function, those arguments don't seem to
> > be necessary. So I'm afraid that the pg_proc entry for the function might
> > be incorrect and those two arguments should be removed from the definition.
> > Is this analysis right?
> 
> Sure looks that way from here.  Copy-and-paste from the previous
> line in pg_proc.h, perhaps?

Yes, that's clearly wrong. Damn.  Can't fix that for 9.5 anymore. The
function isn't all that important (especially not from SQL), but still,
that's annoying.  I'm inclined to just remove the args in 9.6. We could
also add a note to the 9.5 docs, adding that the arguments are there by
error?

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_replication_origin_xact_reset() and its argument variables

[Tom Lane]

Fujii Masao  writes:
> The document explains that pg_replication_origin_xact_reset() doesn't have
> any argument variables. But it's been actually defined so as to have two
> argument variables with pg_lsn and timestamptz data types, as follows.

> =# \df pg_replication_origin_xact_reset
>   List of functions
>Schema   |   Name   | Result data type |
>Argument data types|  Type
> +--+--+--+
>  pg_catalog | pg_replication_origin_xact_reset | void |
> pg_lsn, timestamp with time zone | normal

> As far as I read the code of the function, those arguments don't seem to
> be necessary. So I'm afraid that the pg_proc entry for the function might
> be incorrect and those two arguments should be removed from the definition.
> Is this analysis right?

Sure looks that way from here.  Copy-and-paste from the previous
line in pg_proc.h, perhaps?

regards, tom lane


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

[HACKERS] pg_replication_origin_xact_reset() and its argument variables

[Fujii Masao]

Hi,

The document explains that pg_replication_origin_xact_reset() doesn't have
any argument variables. But it's been actually defined so as to have two
argument variables with pg_lsn and timestamptz data types, as follows.

=# \df pg_replication_origin_xact_reset
  List of functions
   Schema   |   Name   | Result data type |
   Argument data types|  Type
+--+--+--+
 pg_catalog | pg_replication_origin_xact_reset | void |
pg_lsn, timestamp with time zone | normal

As far as I read the code of the function, those arguments don't seem to
be necessary. So I'm afraid that the pg_proc entry for the function might
be incorrect and those two arguments should be removed from the definition.
Is this analysis right?

Regards,

-- 
Fujii Masao


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