Feature #8974 - Declared Local Temporary Tables in Packages#8983
Feature #8974 - Declared Local Temporary Tables in Packages#8983asfernandes wants to merge 6 commits intomasterfrom
Conversation
asfernandes
force-pushed
the
work/packaged-ltt
branch
from
f565c41 to
bcf1e7a
Compare
| } | ||
|
|
||
| if (relationName.package.isEmpty()) | ||
| { // FIXME: indent |
There was a problem hiding this comment.
Forgot to correct? Or is this a reminder for the future?
There was a problem hiding this comment.
It's to be corrected just before the merge or after it.
The indentation makes review difficult.
|
|
||
| MetaName rel_owner_name; // ascii owner | ||
| QualifiedName rel_security_name; // security class name for relation | ||
| bool rel_private = false; |
There was a problem hiding this comment.
Why not use a flag REL_private?
| { $$ = CreateAlterPackageNode::Item::create($2); } | ||
| | PROCEDURE procedure_clause_start ';' | ||
| { $$ = CreateAlterPackageNode::Item::create($2); } | ||
| | DECLARE LOCAL TEMPORARY TABLE package_ltt_table_clause ';' |
There was a problem hiding this comment.
We define functions/procedures (and constants) without any prefix word, so maybe it should be avoided for LTTs too? DECLARE TABLE would be OK inside PSQL modules, as we already have DECLARE {VARIABLE | CURSOR | PROCEDURE | FUNCTION}, but at the package level it looks unnecessary, or at least inconsistent.
There was a problem hiding this comment.
You want it without DECLARE or without DECLARE LOCAL?
There was a problem hiding this comment.
I was thinking about DECLARE being unnecessary, but maybe LOCAL is not needed too, given we don't have GLOBAL packaged temporary tables.
There was a problem hiding this comment.
I understand. That is going to create a narrative problem to fix. DECLARE is redundant: ok. LOCAL is misleading for declarations in header too.
Then this feature is not "Declared LTT" anymore. It's just packaged temporary tables.
When the similar feature exists local to routines, we will need another narrative to join the similar features.
There was a problem hiding this comment.
The name of this feature is not entirely correct.
Here's how I see it.
LOCAL/GLOBAL are about the lifetime of metadata.
Temporary is about the lifetime of data.
Temporary table types:
- Global Temporary Table - Metadata is persistent, data is temporary. Metadata is stored at the schema level. Data scope is connection or transaction.
- Local Temporary Table - Metadata is temporary, data is temporary. Metadata is stored per connection (in memory). Data scope is connection or transaction.
- Packaged Temporary Table - Metadata is persistent, data is temporary. Metadata is stored at the package level. Data visibility is connection or transaction.
- Declared Table - Metadata is persistent, data is temporary. Data visibility is the current subroutine call. Metadata is visible only within the subroutine.
| @@ -1 +1 @@ | |||
| 137 shift/reduce conflicts, 7 reduce/reduce conflicts. | |||
| 138 shift/reduce conflicts, 10 reduce/reduce conflicts. | |||
There was a problem hiding this comment.
I'm worried about new reduce/reduce conflicts, did you analyse the reason?
There was a problem hiding this comment.
The conflicts happens because index are optional and can start with optional UNIQUE, ASC and DESC.
There was a problem hiding this comment.
Maybe change the syntax to
WITH [{[UNIQUE] [ASC | DESC] INDEX <index_name> (<column_name>)}...]
There was a problem hiding this comment.
Yes, [WITH package_ltt_indexes] or [USING package_ltt_indexes] could be a good workaround, IMHO.
There was a problem hiding this comment.
Yes,
[WITH package_ltt_indexes]or[USING package_ltt_indexes]could be a good workaround, IMHO.
This does not resolve the conflicts. It only resolves puting the WITH before each index when there is multiple indexes:
%type package_ltt_index(<createRelationNode>)
package_ltt_index($createRelationNode)
: WITH unique_opt order_direction INDEX valid_symbol_name column_parens
{
const auto node = newNode<CreateIndexNode>(QualifiedName(*$5));
node->unique = $2;
node->descending = $3;
node->columns = $6;
auto clause = newNode<RelationNode::AddPackageLttIndexClause>(node);
$createRelationNode->clauses.add(clause);
}
;
IMO it should not be done. We use btyacc to allow such type of controlled conflicts.
asfernandes
force-pushed
the
work/packaged-ltt
branch
from
dfaa7d5 to
ab02aa5
Compare
5 participants
No description provided.