A problem presentaion about ECPG, DECLARE STATEMENT

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

A problem presentaion about ECPG, DECLARE STATEMENT

kuroda.hayato@fujitsu.com
Dear ALL,

I want to report and consult about DECLARE STATEMENT.
This feature, committed last February, has some bugs.

* This is not thread-independent.
* If some cursors are declared for the same SQL identifier,
  only one cursor you declared at last is enabled.
* This syntax does not have oracle compatibility.

In order to modify bugs, I think many designs, implementations,
and specifications should be changed.
Any operations will be done at the preprocessor process, like #define
macro in C.

It will take about 2 or 3 weeks to make a patch.
Is it acceptable for PG12?

Best Regards,

Hayato Kuroda
FUJITSU LIMITED
E-Mail:[hidden email]



Reply | Threaded
Open this post in threaded view
|

Re: A problem presentaion about ECPG, DECLARE STATEMENT

Michael Meskes-3
Hi Kuroda-san,

> This feature, committed last February, has some bugs.
> ...
> * This syntax does not have oracle compatibility.

This in itself is not a bug. If the syntax is not standard compliant,
then it's a bug. That of course does not mean we would not like to be
Oracle compatible where possible.

> In order to modify bugs, I think many designs, implementations,
> and specifications should be changed.

I hope the authors of said patch speak up and explain why they
implemented it as is.

> Is it acceptable for PG12?

In general absolutely.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL



Reply | Threaded
Open this post in threaded view
|

Re: A problem presentaion about ECPG, DECLARE STATEMENT

Tom Lane-2
Michael Meskes <[hidden email]> writes:
> Hi Kuroda-san,
>> In order to modify bugs, I think many designs, implementations,
>> and specifications should be changed.

> I hope the authors of said patch speak up and explain why they
> implemented it as is.

>> Is it acceptable for PG12?

> In general absolutely.

It seems far too late to be considering any major rewrite for v12;
even assuming that there was consensus on the rewrite being an
improvement, which I bet there won't be.

"Two or three weeks from now" we'll be thinking about pushing 12.0
out the door.  We can't be accepting major definitional changes
at that point.

If a solid case can be made that ECPG's DECLARE STATEMENT was done
wrong, we'd be better off to just revert the feature out of v12
and try again, under less time pressure, for v13.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: A problem presentaion about ECPG, DECLARE STATEMENT

Michael Meskes-3
> > > Is it acceptable for PG12?
> > In general absolutely.
>
> It seems far too late to be considering any major rewrite for v12;
> even assuming that there was consensus on the rewrite being an
> improvement, which I bet there won't be.

Oops, I read 13. Yes, it's obviously way too late for 12. Sorry for the
noise.

In this case I'd like to details about what is wrong with the
implementation.

Thanks.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL



Reply | Threaded
Open this post in threaded view
|

Re: A problem presentaion about ECPG, DECLARE STATEMENT

Peter Eisentraut-6
On 2019-09-11 18:04, Michael Meskes wrote:

>>>> Is it acceptable for PG12?
>>> In general absolutely.
>>
>> It seems far too late to be considering any major rewrite for v12;
>> even assuming that there was consensus on the rewrite being an
>> improvement, which I bet there won't be.
>
> Oops, I read 13. Yes, it's obviously way too late for 12. Sorry for the
> noise.
>
> In this case I'd like to details about what is wrong with the
> implementation.

I tried finding some information about where the idea for this statement
came from but couldn't find any.  The documentation references Oracle
and DB2, and while they indeed do have this statement, it doesn't seem
to be used for the same purpose.  The only purpose in ECPG appears to be
to associate a statement with a connection, but for example the DB2
implementation doesn't even have the AT clause, so I don't see how that
could be the same.

Moreover, I've been wondering about the behavior detail given in the
table at
<https://www.postgresql.org/docs/devel/ecpg-sql-declare-statement.html>.
 In scenario 3, the declare statement says con1 but the subsequent
dynamic statement says con2, and as a result of that, con1 is used.
This is not intuitive, I'd say, but given that there is no indication of
where this statement came from or whose idea it follows, it's unclear
how to evaluate that.

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


Reply | Threaded
Open this post in threaded view
|

RE: A problem presentaion about ECPG, DECLARE STATEMENT

kuroda.hayato@fujitsu.com
Dear all,

Hi, thank you for replying.

> It seems far too late to be considering any major rewrite for v12;

> If a solid case can be made that ECPG's DECLARE STATEMENT was done
> wrong, we'd be better off to just revert the feature out of v12
> and try again, under less time pressure, for v13.

I see, I'll propose this at the next commitfest.
But I'm now considering this commit should be reverted in order to avoid
the confusion.

In oracle and postgres, this statement is used for the purpose of designating
a connection easily. If two functions have a similar goal, these ones should be
used by same way. Some specifications denoted in the document follow oracle's one.
Maybe it's not indicated in the oracle manual, and I understand it should be
discussed more.

Now, one of the major difference of usage between these DBMSs is namespace.
The current namespace unit of postgres is a process, however, oracle ensures
that SQL identifiers are unique only within the file. This means that only
postgres user cannot recycle identifier. This distinction might be inconvenient,
and it makes more confusing to change a namespace after releasing Postgres 12.

I'm now planning remake this function and change namespace unit from a process
to a file.
So I recommend you to throw this away temporally.

I want to hear your opinion.

Best Regards,
Hayato Kuroda
Fujitsu LIMITED

Reply | Threaded
Open this post in threaded view
|

RE: A problem presentaion about ECPG, DECLARE STATEMENT

kuroda.hayato@fujitsu.com
In reply to this post by Peter Eisentraut-6
Dear Peter,

I want to complement about another purpose.
This is that declaring an SQL identifier.

In the oracle (and maybe DB2), the following example is not allowed:

...
EXEC SQL DECLARE cursor CURSOR FOR stmt;
                                  ^^^^
EXEC SQL PREPARE stmt FOR "SELECT ..."
...

This is caused because these preprocessor cannot recognize stmt as an SQL identifier and
throw an error.
I think DB2 might focus on here, so AT clause is not important for them.
But ECPG can accept these sentences, so it has no meaning for postgres.
That is why I did not mention about it and I focused on the omission of AT clause.

Hayato Kuroda
Fujitsu LIMITED

Reply | Threaded
Open this post in threaded view
|

Re: A problem presentaion about ECPG, DECLARE STATEMENT

Tom Lane-2
In reply to this post by kuroda.hayato@fujitsu.com
"[hidden email]" <[hidden email]> writes:
>> If a solid case can be made that ECPG's DECLARE STATEMENT was done
>> wrong, we'd be better off to just revert the feature out of v12
>> and try again, under less time pressure, for v13.

> I see, I'll propose this at the next commitfest.
> But I'm now considering this commit should be reverted in order to avoid
> the confusion.

Per this discussion, I've reverted DECLARE STATEMENT out of v12 and HEAD
branches.

One thing that could use more eyeballs on it is the changes in
ecpg_register_prepared_stmt(); that was added after DECLARE STATEMENT
so there was no prior state to revert to, and I had to guess a bit.
What I guessed, seeing that the lone caller of that function is
already using stmt->connection, was that it was completely bogus
for ecpg_register_prepared_stmt() to be doing its own new connection
lookup and it should just use stmt->connection.  But I might be wrong
since I'm not too clear about where connection lookups are supposed
to be done in this library.

Another comment is that this was one of the more painful reverts
I've ever had to do.  Some of the pain was unavoidable because
there were later commits (mostly the PREPARE AS one) changing
adjacent code ... but a lot of it was due to flat-out sloppiness
in the DECLARE STATEMENT patch, particularly with respect to
whitespace.  Please run the next submission through pgindent
beforehand.  Also please pay attention to the documentation cleanups
that other people made after the initial patch.  We don't want to
have to repeat that cleanup work a second time.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: A problem presentaion about ECPG, DECLARE STATEMENT

Bruce Momjian
In reply to this post by Peter Eisentraut-6
On Mon, Sep 16, 2019 at 01:12:17PM +0200, Peter Eisentraut wrote:
> Moreover, I've been wondering about the behavior detail given in the
> table at
> <https://www.postgresql.org/docs/devel/ecpg-sql-declare-statement.html>.
>  In scenario 3, the declare statement says con1 but the subsequent
> dynamic statement says con2, and as a result of that, con1 is used.
> This is not intuitive, I'd say, but given that there is no indication of
> where this statement came from or whose idea it follows, it's unclear
> how to evaluate that.

FYI, I was totally confused by this also when researching this for the
PG 12 release notes.  I am glad we are going to redo it for PG 13.

--
  Bruce Momjian  <[hidden email]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +