Обсуждение: Problem with insert - 1.12.0 RC1

On Tue, Aug 31, 2010 at 6:19 PM, Sid <sid.the.technician@gmail.com> wrote:
> Hi,
> ~~~~~~~~~~~~~~~~~~~~~~~
> Local installation:
> Windows 7 / Polish
> PgAdmin III: V. 1.12.0 RC1
> PostgreSQL: 8.4.4
> ~~~~~~~~~~~~~~~~~~~~~~~
> Table should have some constraints (not null for instance) .
> Open table to edit rows.
> Put a value into any field and try to move from a row (now I assume that
> field with constraint have some wrong value, null in this case).
> In previous versions PgAdmin shows error message and returns to edit mode.
> In 1.12 it stays in some weird state and all values are lost.
> I reported this bug on pgadmin-support@postgresql.org (on 1.12 beta4) but
> there was no response.
> Can anyone just confirm if it is a bug or I am doing something wrong.

What previous version worked for you? I checked the source back to the
point where we branched 1.10.x, but can't see any changes that are an
obvious cause of such behaviour.


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

Hi Dave,

I can remember that there was a discussion about optimze Log in Server-Status.
Yesterday I saw some wired parse results.
Sample:

begin transaction;
-- doing something ending with a newline
-- doing something else ending with a newline
-- doing something further ending with a newline
insert something with a TIMESTAMP ending with a newline
insert something with a TIMESTAMP ending with a newline
insert something with a TIMESTAMP ending with a newline
insert something with a TIMESTAMP ending with a newline
commit transaction;

Seen is this on a default postgres (no edb) install with default logprefix '%t'.
May be it would be better to tokenize by '\nTIMESTAMP' instead.

Regards Steffen

On Wed, Sep 1, 2010 at 6:50 AM, Steffen Kuhn <pg@kuhnsteffen.de> wrote:
> Hi Dave,
>
> I can remember that there was a discussion about optimze Log in Server-Status.
> Yesterday I saw some wired parse results.
> Sample:
>
> begin transaction;
> -- doing something ending with a newline
> -- doing something else ending with a newline
> -- doing something further ending with a newline
> insert something with a TIMESTAMP ending with a newline
> insert something with a TIMESTAMP ending with a newline
> insert something with a TIMESTAMP ending with a newline
> insert something with a TIMESTAMP ending with a newline
> commit transaction;
>
> Seen is this on a default postgres (no edb) install with default logprefix '%t'.
> May be it would be better to tokenize by '\nTIMESTAMP' instead.

I see the problem - but what about the first line of the file?

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

On Aug 31, 7:19 pm, sid.the.technic...@gmail.com (Sid) wrote:
> Hi,
>
> ~~~~~~~~~~~~~~~~~~~~~~~
> Local installation:
> Windows 7 / Polish
> PgAdmin III: V. 1.12.0 RC1
> PostgreSQL: 8.4.4
> ~~~~~~~~~~~~~~~~~~~~~~~
>
> Table should have some constraints (not null for instance) .
> Open table to edit rows.
> Put a value into any field and try to move from a row (now I assume that
> field with constraint have some wrong value, null in this case).
> In previous versions PgAdmin shows error message and returns to edit mode.
> In 1.12 it stays in some weird state and all values are lost.
>
> I reported this bug on pgadmin-supp...@postgresql.org (on 1.12 beta4) but
> there was no response.
> Can anyone just confirm if it is a bug or I am doing something wrong.

I can confirm the problem for v1.12RC1 and v1.12beta4. I found it just
yesterday but did not have the time to report. All values of a row
vanish after running into an error on save.
The problem is not present in v1.10.5, where the you can keep editing
the row.
It is also noteworthy that it only happens with INSERTs. UPDATEs that
run into an error are not affected.
The problem is rather serious IMO.
Testing on Win XP with pg 8.4.4 on Debian

Regards
Erwin

Le 02/09/2010 00:41, Erwin Brandstetter a écrit :
> On Aug 31, 7:19 pm, sid.the.technic...@gmail.com (Sid) wrote:
>> Hi,
>>
>> ~~~~~~~~~~~~~~~~~~~~~~~
>> Local installation:
>> Windows 7 / Polish
>> PgAdmin III: V. 1.12.0 RC1
>> PostgreSQL: 8.4.4
>> ~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Table should have some constraints (not null for instance) .
>> Open table to edit rows.
>> Put a value into any field and try to move from a row (now I assume that
>> field with constraint have some wrong value, null in this case).
>> In previous versions PgAdmin shows error message and returns to edit mode.
>> In 1.12 it stays in some weird state and all values are lost.
>>
>> I reported this bug on pgadmin-supp...@postgresql.org (on 1.12 beta4) but
>> there was no response.
>> Can anyone just confirm if it is a bug or I am doing something wrong.
>
> I can confirm the problem for v1.12RC1 and v1.12beta4. I found it just
> yesterday but did not have the time to report. All values of a row
> vanish after running into an error on save.
> The problem is not present in v1.10.5, where the you can keep editing
> the row.
> It is also noteworthy that it only happens with INSERTs. UPDATEs that
> run into an error are not affected.

You mean it feels like it added an empty line, right? if this is it, I
can reproduce it. Unfortunately, I didn't find any differences in the
source code between 1.10 and 1.12 that would have this behaviour. If you
confirm me this is the bug you've found, I'll take a deeper look at this.


--
Guillaume
 http://www.postgresql.fr
 http://dalibo.com

  On 02.09.2010 01:14, Guillaume Lelarge wrote:
> Le 02/09/2010 00:41, Erwin Brandstetter a écrit :
>> On Aug 31, 7:19 pm,sid.the.technic...@gmail.com  (Sid) wrote:
>>> Hi,
>>>
>>> ~~~~~~~~~~~~~~~~~~~~~~~
>>> Local installation:
>>> Windows 7 / Polish
>>> PgAdmin III: V. 1.12.0 RC1
>>> PostgreSQL: 8.4.4
>>> ~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Table should have some constraints (not null for instance) .
>>> Open table to edit rows.
>>> Put a value into any field and try to move from a row (now I assume that
>>> field with constraint have some wrong value, null in this case).
>>> In previous versions PgAdmin shows error message and returns to edit mode.
>>> In 1.12 it stays in some weird state and all values are lost.
>>>
>>> I reported this bug onpgadmin-supp...@postgresql.org  (on 1.12 beta4) but
>>> there was no response.
>>> Can anyone just confirm if it is a bug or I am doing something wrong.
>> I can confirm the problem for v1.12RC1 and v1.12beta4. I found it just
>> yesterday but did not have the time to report. All values of a row
>> vanish after running into an error on save.
>> The problem is not present in v1.10.5, where the you can keep editing
>> the row.
>> It is also noteworthy that it only happens with INSERTs. UPDATEs that
>> run into an error are not affected.
> You mean it feels like it added an empty line, right? if this is it, I
> can reproduce it. Unfortunately, I didn't find any differences in the
> source code between 1.10 and 1.12 that would have this behaviour. If you
> confirm me this is the bug you've found, I'll take a deeper look at this.

Yeah, that's exactly what it looks like after pressing 'ok' on the error
msg.: all fields go empty.
Do you want me to create a ticket on trac?

Regards
Erwin

Le 02/09/2010 03:10, Erwin Brandstetter a écrit :
>  On 02.09.2010 01:14, Guillaume Lelarge wrote:
>> Le 02/09/2010 00:41, Erwin Brandstetter a écrit :
>>> On Aug 31, 7:19 pm,sid.the.technic...@gmail.com  (Sid) wrote:
>>>> Hi,
>>>>
>>>> ~~~~~~~~~~~~~~~~~~~~~~~
>>>> Local installation:
>>>> Windows 7 / Polish
>>>> PgAdmin III: V. 1.12.0 RC1
>>>> PostgreSQL: 8.4.4
>>>> ~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> Table should have some constraints (not null for instance) .
>>>> Open table to edit rows.
>>>> Put a value into any field and try to move from a row (now I assume
>>>> that
>>>> field with constraint have some wrong value, null in this case).
>>>> In previous versions PgAdmin shows error message and returns to edit
>>>> mode.
>>>> In 1.12 it stays in some weird state and all values are lost.
>>>>
>>>> I reported this bug onpgadmin-supp...@postgresql.org  (on 1.12
>>>> beta4) but
>>>> there was no response.
>>>> Can anyone just confirm if it is a bug or I am doing something wrong.
>>> I can confirm the problem for v1.12RC1 and v1.12beta4. I found it just
>>> yesterday but did not have the time to report. All values of a row
>>> vanish after running into an error on save.
>>> The problem is not present in v1.10.5, where the you can keep editing
>>> the row.
>>> It is also noteworthy that it only happens with INSERTs. UPDATEs that
>>> run into an error are not affected.
>> You mean it feels like it added an empty line, right? if this is it, I
>> can reproduce it. Unfortunately, I didn't find any differences in the
>> source code between 1.10 and 1.12 that would have this behaviour. If you
>> confirm me this is the bug you've found, I'll take a deeper look at this.
>
> Yeah, that's exactly what it looks like after pressing 'ok' on the error
> msg.: all fields go empty.
> Do you want me to create a ticket on trac?
>

Nope, but thanks :)

I think the issue was added with this commit:

http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commit;h=6b3b661244455951fccbed7895aa22fa7c2ed2a2

In pgadmin/db/pgConn.cpp, method ExecuteSet, the line "return 0;" is
replaced with "return new pgSet();". It fouls the frmEditGrid window,
which thinks it has a valid return, and displays an empty line. I have
no idea why this was changed. ExecuteSet doesn't seem to be used in the
rest of the patch. Dave, can you comment on this please? should we
revert this line to what it was before the patch?


--
Guillaume
 http://www.postgresql.fr
 http://dalibo.com

On Thu, Sep 2, 2010 at 9:16 AM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
> In pgadmin/db/pgConn.cpp, method ExecuteSet, the line "return 0;" is
> replaced with "return new pgSet();". It fouls the frmEditGrid window,
> which thinks it has a valid return, and displays an empty line. I have
> no idea why this was changed. ExecuteSet doesn't seem to be used in the
> rest of the patch. Dave, can you comment on this please? should we
> revert this line to what it was before the patch?

I honestly don't recall what the rationale for the change was. My
*suspicion* is that it was needed to ensure that we get sane behaviour
from all the places that might be calling ExecuteSet in the face of a
failure. Previously, all those objects would break horribly but be
immediately destroyed by the disconnect. Now, they need to survive in
a reasonable state so the connection can be re-established.

My bad for not commenting on it - sometime these things seem so
obvious at the time :-(

Anyway, can the code in the edit grid be modified to detect the empty
pgSet in place of null?

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

Le 02/09/2010 10:28, Dave Page a écrit :
> On Thu, Sep 2, 2010 at 9:16 AM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>> In pgadmin/db/pgConn.cpp, method ExecuteSet, the line "return 0;" is
>> replaced with "return new pgSet();". It fouls the frmEditGrid window,
>> which thinks it has a valid return, and displays an empty line. I have
>> no idea why this was changed. ExecuteSet doesn't seem to be used in the
>> rest of the patch. Dave, can you comment on this please? should we
>> revert this line to what it was before the patch?
>
> I honestly don't recall what the rationale for the change was. My
> *suspicion* is that it was needed to ensure that we get sane behaviour
> from all the places that might be calling ExecuteSet in the face of a
> failure. Previously, all those objects would break horribly but be
> immediately destroyed by the disconnect. Now, they need to survive in
> a reasonable state so the connection can be re-established.
>
> My bad for not commenting on it - sometime these things seem so
> obvious at the time :-(
>

We surely don't comment enough the code.

> Anyway, can the code in the edit grid be modified to detect the empty
> pgSet in place of null?
>

Yeah, by checking how many rows were inserted. See patch attached. It
fixes the issue. Not sure this is the way we should do it. I'm afraid
there are other parts in the code where the "return new pgSet();" code
will bite us.


--
Guillaume
 http://www.postgresql.fr
 http://dalibo.com

On Thu, Sep 2, 2010 at 12:30 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
> Le 02/09/2010 10:28, Dave Page a écrit :
>> Anyway, can the code in the edit grid be modified to detect the empty
>> pgSet in place of null?
>>
>
> Yeah, by checking how many rows were inserted. See patch attached. It
> fixes the issue. Not sure this is the way we should do it. I'm afraid
> there are other parts in the code where the "return new pgSet();" code
> will bite us.

Looks OK to me. I don't expect many other places will have issues - we
probably would have noticed by now.

Besides, fixing occurrences like this is probably much easier than
making reconnect work after an object has unexpectedly been handled a
null pgSet.



--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

Le 02/09/2010 13:59, Dave Page a écrit :
> On Thu, Sep 2, 2010 at 12:30 PM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>> Le 02/09/2010 10:28, Dave Page a écrit :
>>> Anyway, can the code in the edit grid be modified to detect the empty
>>> pgSet in place of null?
>>>
>>
>> Yeah, by checking how many rows were inserted. See patch attached. It
>> fixes the issue. Not sure this is the way we should do it. I'm afraid
>> there are other parts in the code where the "return new pgSet();" code
>> will bite us.
>
> Looks OK to me. I don't expect many other places will have issues - we
> probably would have noticed by now.
>
> Besides, fixing occurrences like this is probably much easier than
> making reconnect work after an object has unexpectedly been handled a
> null pgSet.
>

OK, commited.


--
Guillaume
 http://www.postgresql.fr
 http://dalibo.com