Postgresql TEMP tables grid update problem

[markus]

HI,

i've ran today on problem while trying to use temp tables with grid edition enabled.
Here's my test case:
In TZConnecition int AfterConnect event i perform

Code: Select all

ZConnection1->ExecuteDirect("create temp table temp_test(id serial primary key,\n"
  "i1 integer,\n"
  "i2 integer)");

so i got temp table
now i use TZQuery to do "select * from temp_test"
this query is displayed in standard VCL DBGrid with edition enabled.
Now, when i run two instances of such simple app
I can edit data in app that was run first,
but when i try to post data in second instance i get error "Cannot access temporary tables of other sessions" - but i try to access my own temp table.

I've managed to find a cause of this: in ZDbcPostgreSqlMetadata.pas function UncachedGetColumns() don't check for visibility of table in current session. It was there, but is now commented out
because of Speed decrease: http://zeos.firmos.at/viewtopic.php?p=16646&sid=130
aftere uncommentig this line edition works fine.
Digging some more i found out that when i try to use TZTable in design time i can select temp tables from other sessions.


Best regards,
Marek

[EgonHugeist]

Hi Marek,

sorry for the delay..
May i ask for a faster solution? Do you have an idea how we can get the old behavior back without performance loss?
original thread: http://zeoslib.sourceforge.net/viewtopi ... 46&sid=130

Maybe a faster query with no access to the SP could help. While thinking about ... we could add a parameter to the TZConnection.Properties... But scope of such params is: Connection-Live-Time. I'm affraid in current design i do not see a way for a "flexible-switch".

Cheers, Michael

[EgonHugeist]

Marek,

did a simple patch. Can you tell me if it resolves your issue?

Code: Select all

function TZPostgreSQLDatabaseMetadata.UncachedGetColumns(const Catalog: string;
const SchemaPattern: string; const TableNamePattern: string;
const ColumnNamePattern: string): IZResultSet;
const
nspname_index = {$IFDEF GENERIC_INDEX}0{$ELSE}1{$ENDIF};
relname_index = {$IFDEF GENERIC_INDEX}1{$ELSE}2{$ENDIF};
attname_index = {$IFDEF GENERIC_INDEX}2{$ELSE}3{$ENDIF};
atttypid_index = {$IFDEF GENERIC_INDEX}3{$ELSE}4{$ENDIF};
attnotnull_index = {$IFDEF GENERIC_INDEX}4{$ELSE}5{$ENDIF};
atttypmod_index = {$IFDEF GENERIC_INDEX}5{$ELSE}6{$ENDIF};
attlen_index = {$IFDEF GENERIC_INDEX}6{$ELSE}7{$ENDIF};
attnum_index = {$IFDEF GENERIC_INDEX}7{$ELSE}8{$ENDIF};
adsrc_index = {$IFDEF GENERIC_INDEX}8{$ELSE}9{$ENDIF};
description_index = {$IFDEF GENERIC_INDEX}9{$ELSE}10{$ENDIF};
var
TypeOid, AttTypMod: Integer;
SQL, PgType: string;
SQLType: TZSQLType;
CheckVisibility: Boolean;
ColumnNameCondition, TableNameCondition, SchemaCondition: string;
label CheckColumnsAgain;
begin
CheckVisibility := False;
SchemaCondition := ConstructNameCondition(SchemaPattern,'n.nspname');
TableNameCondition := ConstructNameCondition(TableNamePattern,'c.relname');
ColumnNameCondition := ConstructNameCondition(ColumnNamePattern,'a.attname');
Result:=inherited UncachedGetColumns(Catalog, SchemaPattern, TableNamePattern, ColumnNamePattern);

if (GetDatabaseInfo as IZPostgreDBInfo).HasMinimumServerVersion(7, 3) then
begin
CheckColumnsAgain: //http://zeoslib.sourceforge.net/viewtopi ... 40&t=11174
SQL := 'SELECT n.nspname,' {nspname_index}
+ 'c.relname,' {relname_index}
+ 'a.attname,' {attname_index}
+ 'a.atttypid,' {atttypid_index}
+ 'a.attnotnull,' {attnotnull_index}
+ 'a.atttypmod,' {atttypmod_index}
+ 'a.attlen,' {attlen_index}
+ 'a.attnum,' {attnum_index}
+ 'pg_get_expr(def.adbin, def.adrelid) as adsrc,' {adsrc_index}
+ 'dsc.description ' {description_index}
+ ' FROM pg_catalog.pg_namespace n '
+ ' JOIN pg_catalog.pg_class c ON (c.relnamespace = n.oid) '
+ ' JOIN pg_catalog.pg_attribute a ON (a.attrelid=c.oid) '
+ ' LEFT JOIN pg_catalog.pg_attrdef def ON (a.attrelid=def.adrelid'
+ ' AND a.attnum = def.adnum) LEFT JOIN pg_catalog.pg_description dsc'
+ ' ON (c.oid=dsc.objoid AND a.attnum = dsc.objsubid) '
+ ' LEFT JOIN pg_catalog.pg_class dc ON (dc.oid=dsc.classoid'
+ ' AND dc.relname=''pg_class'') LEFT JOIN pg_catalog.pg_namespace dn'
+ ' ON (dc.relnamespace=dn.oid AND dn.nspname=''pg_catalog'') '
+ ' WHERE a.attnum > 0 AND NOT a.attisdropped';
if SchemaPattern <> '' then
SQL := SQL + ' AND ' + SchemaCondition
else
//not by default: because of Speed decrease: http://http://zeoslib.sourceforge.net/v ... 46&sid=130
if CheckVisibility then
SQL := SQL + ' AND pg_table_is_visible (c.oid) ';
end
else
begin
SQL := 'SELECT NULL::text AS nspname,' {nspname_index}
+ 'c.relname,' {relname_index}
+ 'a.attname,' {attname_index}
+ 'a.atttypid,' {atttypid_index}
+ 'a.attnotnull,' {attnotnull_index}
+ 'a.atttypmod,' {atttypmod_index}
+ 'a.attlen,' {attlen_index}
+ 'a.attnum,' {attnum_index}
+ 'NULL AS adsrc,' {adsrc_index}
+ 'NULL AS description' {description_index}
+ 'FROM pg_class c, pg_attribute a '
+ ' WHERE a.attrelid=c.oid AND a.attnum > 0 ';
end;

If TableNameCondition <> '' then
SQL := SQL + ' AND ' + TableNameCondition;
If ColumnNameCondition <> '' then
SQL := SQL+ ' AND ' + ColumnNameCondition;
SQL := SQL+ ' ORDER BY nspname,relname,attnum';

with GetConnection.CreateStatement.ExecuteQuery(SQL) do
begin
if Next then
repeat
AttTypMod := GetInt(atttypmod_index);

TypeOid := GetInt(atttypid_index);
PgType := GetPostgreSQLType(TypeOid);

Result.MoveToInsertRow;
//Result.UpdateNull(CatalogNameIndex);
Result.UpdateAnsiRec(SchemaNameIndex, GetAnsiRec(nspname_index));
Result.UpdateAnsiRec(TableNameIndex, GetAnsiRec(relname_index));
Result.UpdateAnsiRec(ColumnNameIndex, GetAnsiRec(attname_index));
SQLType := GetSQLTypeByOid(TypeOid);
Result.UpdateInt(TableColColumnTypeIndex, Ord(SQLType));
Result.UpdateString(TableColColumnTypeNameIndex, PgType);

Result.UpdateInt(TableColColumnBufLengthIndex, 0);

if (PgType = 'bpchar') or (PgType = 'varchar') or (PgType = 'enum') then
begin
if AttTypMod <> -1 then
Result.UpdateInt(TableColColumnSizeIndex, GetFieldSize(SQLType, ConSettings, (AttTypMod - 4),
ConSettings.ClientCodePage.CharWidth))
else
if (PgType = 'varchar') then
if ( (GetConnection as IZPostgreSQLConnection).GetUndefinedVarcharAsStringLength = 0 ) then
begin
Result.UpdateInt(TableColColumnTypeIndex, Ord(GetSQLTypeByOid(25))); //Assume text-lob instead
Result.UpdateInt(TableColColumnSizeIndex, 0); // need no size for streams
end
else //keep the string type but with user defined count of chars
Result.UpdateInt(TableColColumnSizeIndex, (GetConnection as IZPostgreSQLConnection).GetUndefinedVarcharAsStringLength )
else
Result.UpdateInt(TableColColumnSizeIndex, 0);
end
else if (PgType = 'numeric') or (PgType = 'decimal') then
begin
Result.UpdateInt(TableColColumnSizeIndex, ((AttTypMod - 4) div 65536)); //precision
Result.UpdateInt(TableColColumnDecimalDigitsIndex, ((AttTypMod -4) mod 65536)); //scale
Result.UpdateInt(TableColColumnNumPrecRadixIndex, 10); //base? ten as default
end
else if (PgType = 'bit') or (PgType = 'varbit') then
begin
Result.UpdateInt(TableColColumnSizeIndex, AttTypMod);
Result.UpdateInt(TableColColumnNumPrecRadixIndex, 2);
end
else
begin
Result.UpdateInt(TableColColumnSizeIndex, GetInt(attlen_index));
Result.UpdateInt(TableColColumnNumPrecRadixIndex, 2);
end;

//Result.UpdateNull(TableColColumnBufLengthIndex);
if GetBoolean(attnotnull_index) then
begin
Result.UpdateString(TableColColumnIsNullableIndex, 'NO');
Result.UpdateInt(TableColColumnNullableIndex, Ord(ntNoNulls));
end
else
begin
Result.UpdateString(TableColColumnIsNullableIndex, 'YES');
Result.UpdateInt(TableColColumnNullableIndex, Ord(ntNullable));
end;

Result.UpdateAnsiRec(TableColColumnRemarksIndex, GetAnsiRec(description_index {description}));
Result.UpdateAnsiRec(TableColColumnColDefIndex, GetAnsiRec(adsrc_index {adsrc}));
//Result.UpdateNull(TableColColumnSQLDataTypeIndex);
//Result.UpdateNull(TableColColumnSQLDateTimeSubIndex);
Result.UpdateInt(TableColColumnCharOctetLengthIndex, Result.GetInt(attlen_index));
Result.UpdateInt(TableColColumnOrdPosIndex, GetInt(attnum_index));

//Result.UpdateNull(TableColColumnAutoIncIndex);
Result.UpdateBoolean(TableColColumnCaseSensitiveIndex, IC.IsCaseSensitive(GetString(attname_index)));
Result.UpdateBoolean(TableColColumnSearchableIndex, True);
Result.UpdateBoolean(TableColColumnWritableIndex, True);
Result.UpdateBoolean(TableColColumnDefinitelyWritableIndex, True);
Result.UpdateBoolean(TableColColumnReadonlyIndex, False);

Result.InsertRow;
until not Next
else //nothing found let's repeat with checking temporary session depended table too.
// notes http://zeoslib.sourceforge.net/viewtopi ... 40&t=11174
if (not CheckVisibility) and (SchemaPattern = '') and (GetDatabaseInfo as IZPostgreDBInfo).HasMinimumServerVersion(7, 3) then
begin
CheckVisibility := True;
Close; //clean up
goto CheckColumnsAgain; //Lest's start the second approach
end;
Close;
end;
end;

[markus]

Hi Michael,

Sorry for not replying for so long - i have almost no time to work at home now... anyway...
i checked latest ZEOS revision (3324) and your patch for uncachedgetcolumns doesn't seem to work.

As i understand it:
in first pass when CheckVisibility := false, there are columns found - but we don't know if they are visible in current session.
That's why loop

Code: Select all

with GetConnection.CreateStatement.ExecuteQuery(SQL) do
  begin
    if Next then
    repeat
...
until not Next

will always have some columns to process, and else clause will never be entered.
At least that's what my simple test application showed.

On the other hand i believe that data integrity is most important - even on cost of speed;)

Best regards,
Marek

[EgonHugeist]

OK Marek, not a problem!

So my patch is wrong... Of course speed isn't everything!

The golden middle-way (we germans say so) ... What about a parameter for TZConnection + PostgresSQL only (using this in TZDataSet-Descendants wouldn't work since the MetaData-Cache isn't related to the IZResultSet-MetaData thing ... internals just some thoughts)?

What do you think?

[markus]

Parameter will be good - yes:)
I was also thinking about conditional define in ZDbcPostgreSqlMetadata.pas to enable
SQL := SQL + ' AND pg_table_is_visible (c.oid) ';

[EgonHugeist]

Done!

R3325 \testing-7.2

add Parameter 'CheckFieldVisibility=True' to the TZConnection.Properties

Hope you test it and give me a reply.

[markus]

Michael,
Patch tested - works fine:)
Thanks a lot


I've also tested patch for issue from http://sourceforge.net/p/zeoslib/tickets/57/, didn't had time to check it earlier, and now ticket is closed.

But when i execute command:

Code: Select all

ZConnection1->ExecuteDirect("alter table temp_test add column \""+Edit1->Text+"\" integer");
ZQuery1->Active = false;
ZQuery1->Active = true;

on live connection i get error "cached plan must not change result type"
i get this error even after calling ZConnection1->DbcConnection->GetMetadata()->ClearCache(); before query refresh - and this used to work ok, if i remember properly.

Best regards,
Marek

[EgonHugeist]

Feel free to RE-open this ticket again! I did close it because nobody was talking to me.

Actually i have some problems to understand you. Do we still run into same issue or is it resolved inbetween (by using the ClearCache-proc -> if so -> alltime code for best performance)?

A second Intention:
A user did provide patch to collect !generic! column-informations about the fetched table by OID. We do cache this Infos because of performance reasons. In the past (after applying the proposed changes) we had a loads of bad criticts and bugreports (if you can understand german language: http://www.lazarusforum.de/viewtopic.php?f=17&t=7691)
See: TZPGTableInfoCache in ZDbcPostgeSQL.pas.

Here i'm willing to ... change the patch to be NOT default, because the actual way isn't 100% safe(for "on the fly altering" table contents" ) IMO even if it helps a loads in a differnet way.

[marsupilami]

Hello Michael,

I seem to remember that particular patch. Maybe it is a good idea to keep the code that fetches the meta data enabled by default but to disable the cache by default and only enable it on request by using a parameter in TZConnection.
I could take a look at these things during the weekend.
Best regards,

Jan

[EgonHugeist]

@Jan,

did this TableCache idea fix your known issues?

If so it might be a good idea to keep the behavior with a parameter as you suggest to do e.g. performance.

What do you think? Can you do this?

[marsupilami]

Hello Michael,

for me the patch fixed two problems: It makes PostgreSQL queries writable where the case of the field named in the query doesn't match the case of the field names in the database (eg ID in the query vs. id in the database). The cache was introduced by me in the hopes that metadata will only have to be queried once and not every time a select statement is executed. As I use PostgreSQL via internet this is vital to me because it makes my application perform faster. So yes - the cache does help me.
I can do the implementation of the parameter. I will make it default to turn the meta data cache off. Is there some point where all the parameters are to be documented so other users will be able to find them?
Best regards,

Jan

[EgonHugeist]

Jan,

ok .. thinking about it a bit more.
As Marek wrote:
[quote=Markus]
on live connection i get error "cached plan must not change result type"
i get this error even after calling ZConnection1->DbcConnection->GetMetadata()->ClearCache(); before query refresh - and this used to work ok, if i remember properly.
[/quote]

So he also needs to clear the build in MetaData-Cache of Zeos. Is there a way to clear you implementation in same call too? Then we could keep default behavior..
Much more interesting would be a ClearCache for the altered objects kind ever...

[markus]

Hi all,

i've found some time to check some more.
Error about changing result set occurs when Query is using real prepared statements. When i added EMULATE_PREPARES=true grid edit on temp tables works, but with clearing cache after each structure change.

It seems that after structure change prepared statement for edited query should be recreated.
Also i think that metadata for table that can be edited in grid should be recreated after each reftesh (at least in postgres, and maybe with some parameter turned on), and yes i'm aware that this might slow things down.
Here's why: plpgsql language can do vitrually anything with database, so any trigger can change structure of almost any table. SO we are not sure if our chache is correct at any time...

Btw Michael, could EMULATE_PREPARES param be also Connection params? - so i don't have to add it to every query i create in runtime (and i do it quite a lot)

Best regards,
Marek

[EgonHugeist]

Marek,

Error about changing result set occurs when Query is using real prepared statements. When i added EMULATE_PREPARES=true grid edit on temp tables works, but with clearing cache after each structure change.

Thats why the STMT is prepared and cached too. One of the performance goals. Simply close the DataSets which accessing the changed table. After your changes are done, the Cache is cleared you can reopen them again. We do NOT cache the Stmts it's selves in a list or so (for updates i'm doing this btw.).
The more i wonder why PG allows doing such things while prepared smts are active and compiled to use such objects!

Note: The more i'm thinking about stop internal ResultSet-recreation and reset cursor positions only. This than avoids the permanental Column-Information determination or buffer allocations(e.g. FireBird/Oracle/ASA) ->goes to 7.3, had not the time to complete the stuff on 7.2.

Also i think that metadata for table that can be edited in grid should be recreated after each reftesh (at least in postgres, and maybe with some parameter turned on), and yes i'm aware that this might slow things down.
Here's why: plpgsql language can do vitrually anything with database, so any trigger can change structure of almost any table. SO we are not sure if our chache is correct at any time...

I do understand your "WHY". But i disagree. This is a developer problem IMHO. Don't know what others say.. For slow PostgreSQL this caching stuff was originally made i think. My personal opinion: I don't like the on the fly altering of Objects in enduser apps, except there is a Software upgade done once or some other one-time administrative reasons. However this is not a discussion about how an appplication should be written and from my point of view, i'm starting from the premisse you know what dou are doing, Marek.

Where i fully agree is: We simply miss a simple way to clear the cache object-wise. You as devoloper should know which cache needs to be cleared and you should have the possibility to do that without clearing everything. Propose you open a ticket as feauture requestt?! 7.3?

Btw Michael, could EMULATE_PREPARES param be also Connection params? - so i don't have to add it to every query i create in runtime (and i do it quite a lot)

done R3400 /testing-7.2 (SVN)