[15721] Implement Sequence in peloton by Kangyan-Zhou · Pull Request #1292 · cmu-db/peloton

Kangyan-Zhou · 2018-04-12T18:36:04Z

This pull request contains the implementation of create sequence.

Right now we are treating sequence as normal transaction as Andy suggested in the project status update meeting.

In this pull request, we modified postgresparser.cpp to support CREATE SEQUENCE statement, with options as defined in pg_sequence(10.0). We currently don't support AS(the third-party pg_query table is not updated yet) and OWNED BY(which we plan to discuss with the other team for namespace issue).

We also created a new file in the catalog, called sequence_catalog.cpp , that can deal with insert, get and delete. In the same file, the SequenceCatalogObject class embeds the functionality of the sequence object.

…r.cpp

coveralls · 2018-04-12T20:00:36Z

Coverage decreased (-0.03%) to 76.935% when pulling 6897566 on HenryZhou0333:master into a9d83a0 on cmu-db:master.

…functioning well

yjc408 · 2018-04-18T23:03:52Z

  TableMetricsCatalog::GetInstance(txn);
  IndexMetricsCatalog::GetInstance(txn);
-  QueryMetricsCatalog::GetInstance(txn);  
+  QueryMetricsCatalog::GetInstance(txn);


It might be better to keep spaces and lines to be consistent as before.

yjc408 · 2018-04-18T23:10:34Z

+
+SequenceCatalog::~SequenceCatalog() {}
+
+/* @brief   Delete the sequence by name.


Inconsistent brief here.

yjc408 · 2018-04-18T23:18:39Z

+ * limit.
+ */
+int64_t SequenceCatalogObject::get_next_val() {
+  int64_t result = seq_curr_val;


Could you add more comments here?

yjc408 · 2018-04-18T23:22:49Z

+              ResultTypeToString(txn->GetResult()).c_str());
+  }
+
+  return (true);


Why it always returns true here?

yjc408 · 2018-04-18T23:34:25Z

+      create_type = CreateType::SEQUENCE;
+      database_name = std::string(parse_tree->GetDatabaseName());
+
+      sequence_name = parse_tree->sequence_name;


Do you still need to assign default values for fields in createStatement if you always assign values to them explictly here?

The value could be empty in the postgres, which leave some field undefined.

lym953 · 2018-04-18T23:43:07Z

+      parser.BuildParseTree(query).release());
+  EXPECT_TRUE(stmt_list->is_valid);
+  if (!stmt_list->is_valid) {
+    LOG_ERROR("Message: %s, line: %d, col: %d", stmt_list->parser_msg,


This seems to be dead code.

yjc408 · 2018-04-19T00:12:26Z

+  bool seq_cycle;         // Whether the sequence cycles
+  concurrency::TransactionContext *txn_;
+
+  std::mutex sequence_mutex;  // mutex for all operations


Maybe unnecessary to have mutex for the current implementation.

Is this mutex being used anywhere? Also, you should use common::synchronization::DirtyMutexLatch

https://github.com/cmu-db/peloton/blob/master/src/include/common/synchronization/mutex_latch.h

yjc408 · 2018-04-19T00:23:28Z

  for (auto &child : children_) {
    child_values.push_back(child->Evaluate(tuple1, tuple2, context));
  }
+  uint64_t ctx = (uint64_t)context;


Already included in ctx. not necessary to cast and pass.

Actually not included, so need to pass it explicitly for sequence functions.

yangjuns

Private functions should be CamelCase; Nextval() Currval() should be in a separate files; I think there are fundamental design problems related to default_database_name.

yangjuns · 2018-04-18T23:48:03Z

            index_name.c_str(), (int)catalog_table_->GetOid());
 }

+/*@brief   Update specific columns using index scan


/*(space)brief
is probably better

yangjuns · 2018-04-18T23:48:44Z

+ * @exception throws SequenceException if the sequence exceeds the upper/lower
+ * limit.
+ */
+int64_t SequenceCatalogObject::get_next_val() {


I think function names should be Camel case, even for private functions.

Yes! Please fix!

yangjuns · 2018-04-18T23:49:46Z

  explicit ExecutorContext(concurrency::TransactionContext *transaction,
-                           codegen::QueryParameters parameters = {});
+                           codegen::QueryParameters parameters = {},
+                           std::string default_database_name = "");


Is this appropriate to add default_database_name here? Even if you want to know the default_database_name when call string_functions, this shouldn't be a separate argument.

I think we need this but the default value for this parameter should not be an empty string. That's going to cause problems later on.

yangjuns · 2018-04-19T00:17:02Z

  static type::Value Lower(const std::vector<type::Value> &args);
+
+  // Sequence-related
+  static type::Value Nextval(const std::vector<type::Value> &args);


These two functions, Nextval() and Currval() should be put into a new sequence file and update binder to bind the file

Correct! These are not string functions!

yangjuns · 2018-04-19T00:31:41Z

                         uint32_t length);
+
+  // Nextval will return the next value of the given sequence
+  static uint32_t Nextval(executor::ExecutorContext &ctx, const char *sequence_name);


These two functions, Nextval() and Currval() should be put into a new sequence file and update binder to bind the file.

Andy suggests to merge the two files as other catalogs.

yangjuns · 2018-04-19T00:32:44Z

  // transform helper for subquery expressions
  static expression::AbstractExpression *SubqueryExprTransform(SubLink *node);

+  static void parse_sequence_params(List *options,


Functions names should be Camel case.

yangjuns · 2018-04-19T00:43:25Z

                         uint32_t length);
+
+  // Nextval will return the next value of the given sequence
+  static uint32_t Nextval(executor::ExecutorContext &ctx, const char *sequence_name);


These two functions don't seem to be tested.

yangjuns · 2018-04-19T00:53:10Z

+#include "catalog/abstract_catalog.h"
+#include "catalog/catalog_defaults.h"
+
+#define SEQUENCE_CATALOG_NAME "pg_sequence"


I think this macro should be declared in catalog_defaults.h.

lym953 · 2018-04-19T00:23:42Z

+                                         const std::string &sequence_name,
+                                         concurrency::TransactionContext *txn) {
+  if (txn == nullptr) {
+    LOG_TRACE("Do not have transaction to drop sequence: %s",


Logging level of LOG_TRACE may be too low here. May consider switching to a higher level, such as LOG_INFO.

This should throw a CatalogException like the other catalog classes.

lym953 · 2018-04-19T00:24:13Z

+  oid_t sequence_oid = SequenceCatalog::GetInstance().GetSequenceOid(
+      sequence_name, database_object->GetDatabaseOid(), txn);
+  if (sequence_oid == INVALID_OID) {
+    LOG_TRACE("Cannot find sequence %s to drop!", sequence_name.c_str());


Logging level of LOG_TRACE may be too low here. May consider switching to a higher level, such as LOG_INFO.

Throw an exception.

lym953 · 2018-04-19T00:30:23Z

+  // the result is a vector of executor::LogicalTile
+  auto result_tiles =
+      GetResultWithIndexScan(column_ids, index_offset, values, txn);
+  // carefull! the result tile could be null!


Typo: "carefull"

lym953 · 2018-04-19T00:33:23Z

+  // the result is a vector of executor::LogicalTile
+  auto result_tiles =
+      GetResultWithIndexScan(column_ids, index_offset, values, txn);
+  // carefull! the result tile could be null!


Typo: "carefull"

lym953 · 2018-04-19T00:37:48Z

+  }
+
+  PELOTON_ASSERT(result_tiles->size() == 1);
+  oid_t result;


Since the variable name "result" does not give any further information, how about compressing lines 293-296 into one line:
return (*result_tiles)[0]->GetValue(0, 0).GetAs<oid_t>();

lym953 · 2018-04-19T00:46:20Z

    std::vector<std::unique_ptr<executor::LogicalTile>> &logical_tile_list) {
  PELOTON_ASSERT(plan != nullptr);
  LOG_TRACE("PlanExecutor Start with transaction");
+  LOG_DEBUG("PlanExecutor 2");


It seems you forgot to delete this line :)

lym953 · 2018-04-19T00:50:02Z

 }

+type::Value OldEngineStringFunctions::Nextval(
+        UNUSED_ATTRIBUTE const std::vector<type::Value> &args) {


"UNUSED_ATTRIBUTE" may be removed.

lym953 · 2018-04-19T00:50:22Z

+}
+
+type::Value OldEngineStringFunctions::Currval(
+        UNUSED_ATTRIBUTE const std::vector<type::Value> &args) {


"UNUSED_ATTRIBUTE" may be removed.

lym953 · 2018-04-19T00:59:21Z

  bool    if_not_exists;  /* just do nothing if schema already exists? */
 } CreateSchemaStmt;

+typedef struct CreateSeqStmt


Not properly formatted

lym953 · 2018-04-19T01:01:37Z

+void PostgresParser::parse_sequence_params(List *options,
+                                           parser::CreateStatement *result) {
+  DefElem *start_value = NULL;
+  // DefElem    *restart_value = NULL;


apavlo · 2018-06-21T13:32:40Z

Replaced by #1407

Kangyan-Zhou and others added 19 commits March 22, 2018 16:07

finish part of parser; need to add handler function in create_executo…

30d48ac

…r.cpp

finish part of parser; need to add handler function in create_executo…

086b493

…r.cpp

Merge branch 'master' of https://github.com/HenryZhou0333/peloton

59b85f6

Merge remote-tracking branch 'upstream/master'

282afb3

add postgresql parser test for sequences

28b702e

remove AS in sequencce

ddb3dd7

add exception for redefined parser

26230e3

finish compiling create sequence

b273056

Merge remote-tracking branch 'upstream/master'

d90a649

having issue with GetSequence()

c1a4e1d

add new GetSequence impl; fix BasicTest in sequence

f8ec42a

add tests for sequence

0d3521c

fix merge conflict

8fe7d6c

one step before submitting pr

87680be

change way of update pg_sequence table in nextval

7d7955c

style fix

503c89f

rename sequence test

7762354

Merge remote-tracking branch 'upstream/master'

b340c86

remove unnecessary file

6897566

apavlo added do not merge class-project labels Apr 12, 2018

danae-s and others added 2 commits April 14, 2018 17:59

adding nextval & currval functions; comment out lock and nextval not …

b8c7317

…functioning well

update pg_sequence after calling nextval; have seg fault in txn;

d665c94

yjc408 reviewed Apr 18, 2018

View reviewed changes

lym953 reviewed Apr 18, 2018

View reviewed changes

yjc408 reviewed Apr 19, 2018

View reviewed changes

yangjuns reviewed Apr 19, 2018

View reviewed changes

lym953 reviewed Apr 19, 2018

View reviewed changes

apavlo closed this Jun 21, 2018


		SequenceCatalog::~SequenceCatalog() {}

		/* @brief Delete the sequence by name.

Conversation

Kangyan-Zhou commented Apr 12, 2018

Uh oh!

coveralls commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjc408 Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangjuns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjc408 Apr 19, 2018 •

edited

Loading